615870 - Remote HTML5 video rendering pipeline should be shorter

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: PuppetWidget::Invalidate backtrace

I've been testing HTML5 video rendering pipeline in remote fennec (content process side... ) and rendering/invalidate backtrace's looks a bit long and bring lot of additional functions calls in profile.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Video Paint backtrace

Comment 2

[Oleg Romashin (:romaxa)]

Not sure, but should not it be bit shorter, like:
1) MediaDecoder GetImageLayer()->Update() (push video frame in image layer backBuffer)
2) Send Buffer to chrome process
3) Swap / Upload to GPU

And do it as fast as possible.

Do we have any bug or work planned to improve this rendering pipeline

Comment 3

[Oleg Romashin (:romaxa)]

Also I found that we rotating event loop between NotifyInvalidation and PuppetWidget::Paint should it be faster for video content update layer immediately ?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Work is in-progress, bug 598868.  We don't immediately dispatch paints on invalidates because I don't think that's safe in general.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Make paint sync for Video and Plugins

Of course this patch still need some tweaking about to do that only for Content Process et.c. but I would like to hear some comment about can I use INVALIDATE_IMMEDIATE flag for this case (and probably kill Composite functionality) or it is better create new flag?

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 494415 [details] [diff] [review]
Make paint sync for Video and Plugins

ah and this gives ~2-3 FPS for flash rendering... (16-17->20 FPS)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Making paints sync is not the right way to go. Sync paint is dangerous. It messes up frame rate control. And it's actually going to be slower in many cases because it reduces paint coalescing. E.g., if you combine multiple videos with animations, you're going to paint far too often.

Comment 8

[Oleg Romashin (:romaxa)]

> with animations, you're going to paint far too often.
ok, I see.

But I see two problems here:
 - right now on plugin or video update we do calculate Invalidation Rect for video/plugin layer, (~18 calls https://bug615870.bugzilla.mozilla.org/attachment.cgi?id=494379) and then trying to build from Invalidation Rect DisplayList (another bunch of heavy functions) and finally reaching our layer...

Can we do it somehow faster and on layer update don't calc and pass InvalidateRegion, but mark updated layer region inside layer and send some list of layers through Invalidate/Paint pipeline?

 - Another problem is that current ImageLayer API allow us to set data only using ImageContainer and images, and don't allow us to call some function like imageLayer->SetData(), and update ShadowableImageLayer mBackBuffer directly, direct composite from yuv -> mBackBuffer.
I think it would be nice to have such API and use it in shadow layers update

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> But I see two problems here:
>  - right now on plugin or video update we do calculate Invalidation Rect for
> video/plugin layer, (~18 calls
> https://bug615870.bugzilla.mozilla.org/attachment.cgi?id=494379) and then
> trying to build from Invalidation Rect DisplayList (another bunch of heavy
> functions) and finally reaching our layer...

Bug 598868 (and a similar bug for plugins) will bypass this.

> Can we do it somehow faster and on layer update don't calc and pass
> InvalidateRegion, but mark updated layer region inside layer and send some list
> of layers through Invalidate/Paint pipeline?

Not exactly. We can update the current image and recomposite the current layer tree, assuming nothing else has changed. This is not trivial to get right, though; not something anyone is likely to be able to quickly hack in.

>  - Another problem is that current ImageLayer API allow us to set data only
> using ImageContainer and images, and don't allow us to call some function like
> imageLayer->SetData(), and update ShadowableImageLayer mBackBuffer directly,
> direct composite from yuv -> mBackBuffer.
> I think it would be nice to have such API and use it in shadow layers update

I don't understand what the problem is. The existing API should be fine. Some work is needed to make ImageContainer::SetCurrentImage update the compositor's copy of the layer tree directly; that work is bug 598868, and shouldn't require API changes. (Going through ImageContainer instead of ImageLayer is actually the right thing; ImageLayers aren't threadsafe but ImageContainer is, so the video decoder thread can call ImageContainer::SetCurrentImage directly without having to sync with the main thread.)

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #8)
>  - Another problem is that current ImageLayer API allow us to set data only
> using ImageContainer and images, and don't allow us to call some function like
> imageLayer->SetData(), and update ShadowableImageLayer mBackBuffer directly,
> direct composite from yuv -> mBackBuffer.
> I think it would be nice to have such API and use it in shadow layers update

Are you talking about avoiding this code? -- http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayers.cpp#1755 .  There's bug 580781, but it's not worth investing too much time+code in it because it will be obsoleted by bug 598868.  (I *almost* WONTFIX'd it last week.)  That said, I'm open to relatively noninvasive, temporary patches before 598868 is ready though.

Comment 11

[Oleg Romashin (:romaxa)]

> Not exactly. We can update the current image and recomposite the current layer
> tree, assuming nothing else has changed. This is not trivial to get right,
> though; not something anyone is likely to be able to quickly hack in.

Wow! I did quick hack, as you explained here, and got 30FPS for http://www.youtube.com/watch?v=JsujaaB4x4w video + ~20% CPU idle

Comment 12

[Oleg Romashin (:romaxa)]

http://camendesign.com/code/video_for_everybody/test.html - 25FPS + ~45 CPU idle

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Mega Video and Plugins speedup

Update image and composite current tree...
This make youtube video 30FPS + 15% CPU idle

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: A bit hacky, but works more correct

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This approach can work. The idea of having a flag in the widget which tells us whether a recomposite is sufficient is good. Letting the layer manager tell us whether the composite-only succeeded is good too, although I think it would make more sense for EndTransaction to return a value directly.

Obviously we'd need to change Invalidate to take a flags word.

+      if (!aCallback && aLayer->GetType() == Layer::TYPE_THEBES && child->GetType() == Layer::TYPE_THEBES) {

This is weird ... this condition can't possibly be true, ThebesLayers never have children. I think you should just locate the places in ThebesLayer that want to call into aCallback, and propagate failure out from there. The best approach might be to add a flag to the layer manager itself to detect if the recomposite succeeded.

However, it seems to me that bug 598868 will fix most of this. I think we should focus on getting that done and landed and then build on that.

Comment 16

[Oleg Romashin (:romaxa)]

> +      if (!aCallback && aLayer->GetType() == Layer::TYPE_THEBES &&
> child->GetType() == Layer::TYPE_THEBES) {

I'm getting this true on www.aa.com

But yes return fail on attempt to call aCallback and make EndTransaction return value would be more clear solution here

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #16)
> > +      if (!aCallback && aLayer->GetType() == Layer::TYPE_THEBES &&
> > child->GetType() == Layer::TYPE_THEBES) {
> 
> I'm getting this true on www.aa.com

Er, can you dump that frame tree?

Comment 18

[Oleg Romashin (:romaxa)]

> 
> Er, can you dump that frame tree?

Sorry I was wrong, you right

> However, it seems to me that bug 598868 will fix most of this. I think we
> should focus on getting that done and landed and then build on that.

I think 598868 fixing reviewing, landing will take some time... also we still need implementation for new IPC Protocol API (video/plugins)...

I think it would be nice to ship first Fennec Release with really fast HTML5 video support (and probably flash on Maemo5), and question is it really possible to get 598868 in first Fennec release?

Comment 19

[Oleg Romashin (:romaxa)]

With this bug + 616469 we can ship really fast video/flash for Fennec on android/Maemo5

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Updated, still without EndTransaction return value change

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #18)
> I think it would be nice to ship first Fennec Release with really fast HTML5
> video support (and probably flash on Maemo5), and question is it really
> possible to get 598868 in first Fennec release?

I don't know. But I hate doing "temporary fixes" for a release that mean more work for later releases.

Comment 22

[Oleg Romashin (:romaxa)]

yes you partially right... but I hate see mozilla features working slow.
Also I see this fix more like optimization for current implementation.

also problems with new implementations that they are usually landed such way that it works and does not break anything and first implementation usually landed without any optimizations :(

So I guess 598868 will take more time before it works right and fast.
Fix me if I'm wrong...

Anyway I'll spend a little time for making this approach working good. and we can take it if 598868 not ready.

Comment 23

[Oleg Romashin (:romaxa)]

Also thinking about temporary fix... does it make sense to change EndTransaction for short term optimization?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I'm not really worried about that.

Comment 25

[Oleg Romashin (:romaxa)]

Attachment: EndTransaction return value

Updated. added retval for EndTransaction, 
Also added transaction error because I need to detect errors coming from Paint function... and I found that it is a bit tricky to change that returning bool value, and make difference between real paint errors or errors with null callback...
Any suggestions?
Also not sure how to make better this hack:

aWidget->Invalidate(bounds, aUpdateFlags & NS_VMREFRESH_ONLY_VIDEO ? 2 : PR_FALSE);

Don't want to update API... and break Sync flag functionality...

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      && GeckoProcessType_Default != XRE_GetProcessType()) {

Why would we only do this for content processes? If we're going to do this, we should do it for all processes, desktop too. We need to minimize divergence.

+  if (aFlags & INVALIDATE_ONLY_VIDEO_LAYERS)
+    flags |= NS_VMREFRESH_ONLY_VIDEO;

{}

Maybe instead of doing this through nsIWidget we should do it in nsIPresShell::Paint. The nsRootPresContext can hold the mPendingVideoOnlyUpdate flag ... but we should call it something like mUpdateLayerTree (with reversed meaning, so it starts of false and flips to true whenever an Invalidate happens that doesn't have INVALIDATE_ONLY_VIDEO_LAYERS. Except we should call that flag INVALIDATE_NO_UPDATE_LAYER_TREE. PresShell::Paint can check mUpdateLayerTree and do the layer tree transaction shortcut.

This way, it would work across all widget backends automatically and you'll have to update less code.

Comment 27

[Oleg Romashin (:romaxa)]

Attachment: Moved out from Widget

Ok moved into nsRootPresContext..
SetIsUpdateLayerTree - still a bit hacky... and possible should be enum value or something...

roc what should I use here for SetIsUpdateLayerTree, and mUpdateLayerTree enum? or current logic is wrong (it works fine)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  virtual bool EndTransaction(DrawThebesLayerCallback aCallback,
                               void* aCallbackData) = 0;

We'll want to document the meaning of the return value. (And what happens when you pass a null callback).

+  void SetIsUpdateLayerTree(PRUint32 aUpdate) { mUpdateLayerTree = aUpdate; }
+  PRUint32 IsUpdateLayerTree() { return mUpdateLayerTree; }

SetNeedToUpdateLayerTree
NeedToUpdateLayerTree

should just be a boolean. When it's false, you don't need to update the layer tree so you can just do BeginTransaction/EndTransaction.

The current logic is wrong, I think you're being lucky. We need to ensure that mUpdateLayerTree is set for every kind of invalidation other than InvalidateLayer on a plugin or video.

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: Should be right

Yep yesterday I was doing wrong things and was tired a bit...
Hopefully now it should be better

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Instead of mTransactionError I think we should call it mTransactionIncomplete or something like that. It's not really an error.

D3D9/D3D10/GL EndTransaction will need to be updated to return real values now.

+  , mUpdateLayerTree(false)

comma at end of previous line

+  nsRootPresContext* rootPC = PresContext()->GetRootPresContext();
+  if (aFlags != INVALIDATE_NO_UPDATE_LAYER_TREE) {
+    rootPC->SetNeedToUpdateLayerTree(true);
+  }

Move the getting of rootPC into the if body. Share the call to PresContext() where we get the PresShell above.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is pretty close. It looks nice.

Once it lands we should see if it improves performance for tests that just play video.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh also, aFlags != INVALIDATE_NO_UPDATE_LAYER_TREE should be "if (!(aFlags & INVALIDATE_NO_UPDATE_LAYER_TREE))"

Comment 33

[Oleg Romashin (:romaxa)]

Attachment: Fixed comments + mRoot check

Fixed crash on first paint when mRoot is not set yet.
Checked on Qt N900

N900 (SW rendering) youtube 7FPS -> 17FPS

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> Instead of mTransactionError I think we should call it mTransactionIncomplete
> or something like that. It's not really an error.

Please make this change.

> D3D9/D3D10/GL EndTransaction will need to be updated to return real values now.

And this one.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should work for TYPE_CANVAS too.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you don't want to actually implement the EndTransaction abort properly for all LayerManagers, that's OK, but returning 'true' to pretend you succeeded even if you didn't will break everything. Instead you should probably add a method to LayerManager to indicate if it supports null callbacks and the EndTransaction return value.

Comment 37

[Oleg Romashin (:romaxa)]

Attachment: Hopefully updated

Updated, mTransactionIncomplete and disable transaction if manager is non-basic...

Comment 38

[Oleg Romashin (:romaxa)]

Attachment: IsNullTransactionSupported method added + CANVAS

Comment 39

[Oleg Romashin (:romaxa)]

Comment on attachment 496939 [details] [diff] [review]
IsNullTransactionSupported method added + CANVAS

Ok, this version is bad due to this crash
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292036845.1292037120.19829.gz#err0

Need to check for that aCallback too

Comment 40

[Oleg Romashin (:romaxa)]

Attachment: This version seems green enough

http://hg.mozilla.org/try/rev/24c9e1cec1d7

Comment 41

[Oleg Romashin (:romaxa)]

Attachment: Drop canvas element...

Canvas does not work. push it without canvas

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

IsNullTransactionSupported at least needs a comment to explain what a "null transaction" is.

SetTransactionError should be SetTransactionIncomplete.

+  bool IsNullTransactionSupported() { return mRoot ? true : false; }

mRoot != nsnull

Why didn't canvases work?

Comment 43

[Oleg Romashin (:romaxa)]

Attachment: mRoot != nsnull

Canvas did not worked because in order to update canvas we  should call BuildLayer, which supposed to call nsHTMLCanvasElement::GetCanvasLayer and call Update (with ReadPixels into actual remote canvas Buffer)

But with direct transaction we do not call FrameBuilder et.c.

Comment 44

[Oleg Romashin (:romaxa)]

Attachment: Right patch, fixed description and mRoot != nsnull

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * If aCallback is null, this is a 'null' transaction.
+   * There must have been no updates to the layer tree in this transaction.
+   * A null transaction can fail, in which case EndTransaction returns false,
+   * and the transaction must be retried with aCallback set to something non-null.

Need extra line:
    * If IsNullTransactionSupported() returns false, then aCallback must be non-null.

> SetTransactionError should be SetTransactionIncomplete.

You didn't do this.

Comment 46

[Oleg Romashin (:romaxa)]

Attachment: SetTransactionIncomplete, + minor comment update

> You didn't do this.
oh sorry,

Comment 47

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/34bd12eb4a9c

Comment 48

[Michael Wu [:mwu]]

Sorry - backed this out for causing bug 618916 .

Comment 49

[Oleg Romashin (:romaxa)]

Ok,  sent try build with this patch + patch from 618807 + patch from 618730

http://hg.mozilla.org/try/rev/8db82ab1b2d0

Please check 618916, because I'm not able to reproduce it, even with current patch only.

Comment 50

[Oleg Romashin (:romaxa)]

Attachment: Bugs 618807, 618832, 618730

Comment 51

[Oleg Romashin (:romaxa)]

Test build available here
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8db82ab1b2d0/

Comment 52

[Ed Morley [:emorley]]

(In reply to comment #47)
> http://hg.mozilla.org/mozilla-central/rev/34bd12eb4a9c
Initial landing on m-c is believed to have caused bug 618941 also.

Comment 53

[Oleg Romashin (:romaxa)]

Comment on attachment 497430 [details] [diff] [review]
Bugs 618807, 618832, 618730

mbrubeck tested build with this patch, and don't see any problems with fennec UI anymore.
roc: you already set r+ for patches in other bugs.. but they are fixed.. and tree closed... would be nice to r+ it again

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you check that bug 618941 is fixed by the new patches?

I would like to know why bug 618807 was caused by the original patch here. Do you understand what went wrong and why your patch fixes it?

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also seems to have caused topcrash bug 619060.

Comment 56

[Oleg Romashin (:romaxa)]

(In reply to comment #54)
> Can you check that bug 618941 is fixed by the new patches?
I'll check this

> you understand what went wrong and why your patch fixes it?

I just found that when popup window appearing we are getting InvalidateFlags with empty damage rect... why that empty I don't know should I get backtrace or you do have better proposals?


(In reply to comment #55)
> Also seems to have caused topcrash bug 619060.

is there are any steps to reproduce it?

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #56)
> I just found that when popup window appearing we are getting InvalidateFlags
> with empty damage rect... why that empty I don't know should I get backtrace or
> you do have better proposals?

A backtrace would be great.

Comment 58

[Oleg Romashin (:romaxa)]

bug 618941 is fixed by these patches.

Comment 59

[Oleg Romashin (:romaxa)]

Attachment: Backtrace for empty rect

Ok, damage rect is empty and has these values
86880,0,0,30540
0,30540,86880,0

And backtrace in attachment

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK so we're doing a reflow but nothing's really changing. We get an empty invalidation here because the reflow root (the popup perhaps) has not changed size. I don't think relying on that invalidation to trigger display list processing is the right thing.

Instead, I think hiding a popup (combobox dropdown or menu/XUL panel) should invalidate everything in the popup so next time it's displayed we'll definitely refresh it. This would mean calling nsIFrame::InvalidateFrameSubtree from nsComboboxControlFrame::ShowPopup an dnsMenuPopupFrame::HidePopup.

Comment 61

[Oleg Romashin (:romaxa)]

Should we track in this bug still?

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes I think so, unless you want to file a bug that blocks this one.

Comment 63

[Oleg Romashin (:romaxa)]

Attachment: this was r+ before, no damage rect fix

Comment 64

[Oleg Romashin (:romaxa)]

Attachment: call InvalidateFrameSubtree from comboboxShowPopup, and menu hidePopup

tried this, but it did not help... should I call shell->rootFrame->invalidateSubTree or  something else is wrong?

Comment 65

[Oleg Romashin (:romaxa)]

Attachment: Workaround fix, for empty damage rect invalidate only

Will keep it as separate patch

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try calling InvalidateFrameSubtree from nsMenuPopupFrame::ShowPopup as well.

Comment 67

[Oleg Romashin (:romaxa)]

Attachment: This help for CSM "Copy" menu, but not for autocompletion 618941

Comment 68

[Oleg Romashin (:romaxa)]

Attachment: damageRect[0,0,0,0], flags:50

ok, for autocompl menu it is coming from FrameLayerBuilder::DrawThebesLayer, FrameLayerBuilder.cpp:1592

Comment 69

[Oleg Romashin (:romaxa)]

Attachment: Visibility Break

Comment 70

[Oleg Romashin (:romaxa)]

Attachment: Visibility break, from nsMenuPopupFrame::LayoutPopup

Comment 71

[Oleg Romashin (:romaxa)]

Attachment: DumpPaintList + LayerTree dump.

On line 729 - ShowPopup call.
Func:void nsMenuPopupFrame::ShowPopup(PRBool, PRBool)::770 size[0,0]
Func:InvalidateWithFlags::4048 r[0,0,0,0], flags[0]
Func:InternalInvalidateThebesLayersInSubtree::1455 NS_FRAME_HAS_CONTAINER_LAYER:0
Func:InternalInvalidateThebesLayersInSubtree::1463 NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT:0
Func:InvalidateWithFlags::4048 r[137040,0,0,37080], flags[0]
Func:InvalidateWithFlags::4048 r[0,37080,137040,0], flags[0]

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The original patch has problems if the transaction fails. We bail out of PaintLayer without cleaning up properly. I'll fix that.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK I think the cause of the problem with the autocomplete dropdown (and maybe other dropdowns) is that there are multiple widgets and layer managers in play here all sharing the same rootprescontext. So what happens is that we do some invalidates and then get a paint for the main window followed by a paint for the dropdown. The paint for the main window does SetNeedToUpdateLayerTree(false) and then the paint for the dropdown thinks it can just do a null transaction.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I will try to fix this tomorrow, or maybe tonight. I think the solution is to change the flag to a "layer tree up to date" flag in the layer manager userdata (LayerManagerData), and clear it in InvalidateRoot for the retained layer manager on the frame's widget.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Layers "null transaction" API

Splitting out the layers changes ... cjones, I'm suspicious of the BasicShadowLayerManager::EndTransaction change here. Calling ShadowLayerForwarder::EndTransaction(nsnull) for an incomplete transaction will cause flicker in the compositor process, won't it? The failed EndTransaction will be followed by a real transaction with non-null aCallback, which can't fail, so is there a way to extend or abort the ShadowLayerForwarder transaction?

I wonder if it would be better API to not call BeginTransaction again. Instead, a failed EndTransaction(nsnull, nsnull) would have to be followed by another EndTransaction with non-null aCallback, no intervening BeginTransaction.

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: track per-display-root-frame "needs layer tree update" state bit

This seems better.

Oleg, please test these patches to make sure they work for you.

Comment 77

[Oleg Romashin (:romaxa)]

> 
> Oleg, please test these patches to make sure they work for you.
Tested your version, works fine for me. video is fast and smooth

Comment 78

[Timothy Nikkel (:tnikkel)]

Comment on attachment 498710 [details] [diff] [review]
Part 2: track per-display-root-frame "needs layer tree update" state bit

>@@ -4251,16 +4257,20 @@ nsIFrame::InvalidateRoot(const nsRect& a
>+  if (!(aFlags & INVALIDATE_NO_UPDATE_LAYER_TREE)) {
>+    AddStateBits(NS_FRAME_UPDATE_LAYER_TREE);
>+  }

So we are not going to get the NS_FRAME_UPDATE_LAYER_TREE bit for invalidates that come from resizing or changing the visibility of views.

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Correct.

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think it matters; I think anything that resizes views is also going to invalidate normally.

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 498709 [details] [diff] [review]
Part 1: Layers "null transaction" API

I think we can actually improve the API here. Instead of using a null aCallback and the IsNullTransactionSupported() method, I think we should just have a new virtual method DoNullTransaction() which does effectively BeginTransaction(); EndTransaction(nsnull,nsnull) and returns true if it succeeded. So the default implementation would just return false without doing anything else.

Comment 82

[Timothy Nikkel (:tnikkel)]

(In reply to comment #80)
> I don't think it matters; I think anything that resizes views is also going to
> invalidate normally.

I debugged at least one case where that wasn't true (but is now). So if you're confident we can go this way.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What was that case?

Would you be more comfortable if we had reflows always set the "needs layer tree update" flag?

Comment 84

[Timothy Nikkel (:tnikkel)]

Bug 540247 was what I was thinking of. Just scanning the source for deck frames I'm not sure if they do frame invalidates everywhere they should.

But if you are fine with your current approach thats ok, if we see problems we'll know why quickly.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 v2

I think this DoEmptyTransaction API better.

This patch fixes potential issues with ShadowLayers too. If the transaction is incomplete we don't forward our update list to the parent, we let the next transaction to append to the update list and forward it.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #84)
> Bug 540247 was what I was thinking of. Just scanning the source for deck frames
> I'm not sure if they do frame invalidates everywhere they should.
> 
> But if you are fine with your current approach thats ok, if we see problems
> we'll know why quickly.

Decks call Redraw() when the index changes, which invalidates the entire frame subtree.

nsDeckFrame::DoLayout shows and hides views, but off the top of my head I can't think of a case where we need to do special invalidation in that case. If we do, it's already broken since ThebesLayers would need to be updated and showing and hiding views doesn't do that.

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

Updated for the DoEmptyTransaction API.

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oleg, you probably should test these patches again.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1 v3

Oops, turns out we use null callbacks in other situations --- when we're setting up the layer tree for a temporary layer manager and not painting anything yet.

Comment 90

[Oleg Romashin (:romaxa)]

> Part 1 v3 > Part 2 v2
Tested, youtube and URL is fast

Comment 91

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 498987 [details] [diff] [review]
Part 1 v3

> Calling
> ShadowLayerForwarder::EndTransaction(nsnull) for an incomplete transaction will
> cause flicker in the compositor process, won't it?

Yes.

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp
>@@ -412,16 +412,20 @@ protected:
> 
>   virtual void
>   PaintBuffer(gfxContext* aContext,
>               const nsIntRegion& aRegionToDraw,
>               const nsIntRegion& aRegionToInvalidate,
>               LayerManager::DrawThebesLayerCallback aCallback,
>               void* aCallbackData)
>   {
>+    if (!aCallback) {
>+      BasicManager()->SetTransactionIncomplete();
>+      return;
>+    }

This shouldn't be necessary, because we only get here from
BasicThebesLayer::Paint(), which already has this check.  A hard
|assert(aCallback)| would be useful though.

Looks good to me.

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #91)
> This shouldn't be necessary, because we only get here from
> BasicThebesLayer::Paint(), which already has this check.  A hard
> |assert(aCallback)| would be useful though.

The check in BasicThebesLayer::Paint is not on the path that leads to PaintBuffer. So I think we still need this check in PaintBuffer.

Comment 93

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Duh, right.  There might be some value in keeping these checks localized to Paint() but I don't care so much.

Comment 94

[Oleg Romashin (:romaxa)]

Tried these patches and got next errors:
TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug348236.html
TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_menuchecks.xul | Exited with code 1 during test run
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul | Exited with code 1 during test run
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1293195464.1293196014.24513.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1293195461.1293195895.23414.gz

Comment 95

[Oleg Romashin (:romaxa)]

Attachment: Check for mRoot, fixing crash-oranges...

try build
http://hg.mozilla.org/try/rev/1bf8037d7854

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This allows DoEmptyTransaction to return true if there's no mRoot. I think that's wrong; if mRoot is false, DoEmptyTransaction should return false immediately.

Comment 97

[Oleg Romashin (:romaxa)]

Attachment: return false immediately if !mRoot

Comment 98

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/a07894326d5d
http://hg.mozilla.org/mozilla-central/rev/0a7d57ee6a0a