Closed Bug 615870 Opened 9 years ago Closed 9 years ago

Remote HTML5 video rendering pipeline should be shorter

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: romaxa, Assigned: romaxa)

References

(Depends on 1 open bug, )

Details

Attachments

(8 files, 26 obsolete files)

4.98 KB, text/plain
Details
4.72 KB, text/plain
Details
3.06 KB, text/plain
Details
560.38 KB, text/plain
Details
6.86 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.75 KB, patch
Details | Diff | Splinter Review
11.73 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
Attached file Video Paint backtrace
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
Also I found that we rotating event loop between NotifyInvalidation and PuppetWidget::Paint should it be faster for video content update layer immediately ?
Work is in-progress, bug 598868.  We don't immediately dispatch paints on invalidates because I don't think that's safe in general.
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?
Attachment #494415 - Flags: feedback?(roc)
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)
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.
> 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
(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.)
(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.
> 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
Attached patch Mega Video and Plugins speedup (obsolete) — Splinter Review
Update image and composite current tree...
This make youtube video 30FPS + 15% CPU idle
Attachment #494415 - Attachment is obsolete: true
Attachment #495356 - Flags: feedback?(roc)
Attachment #494415 - Flags: feedback?(roc)
Attachment #495356 - Attachment is obsolete: true
Attachment #495397 - Flags: feedback?(roc)
Attachment #495356 - Flags: feedback?(roc)
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.
> +      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
(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?
> 
> 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?
With this bug + 616469 we can ship really fast video/flash for Fennec on android/Maemo5
Attachment #495397 - Attachment is obsolete: true
Attachment #495397 - Flags: feedback?(roc)
tracking-fennec: --- → 2.0+
(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.
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.
Also thinking about temporary fix... does it make sense to change EndTransaction for short term optimization?
Yes, I'm not really worried about that.
Attached patch EndTransaction return value (obsolete) — Splinter Review
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...
Attachment #495845 - Attachment is obsolete: true
Attachment #496017 - Flags: feedback?(roc)
+      && 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.
Attached patch Moved out from Widget (obsolete) — Splinter Review
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)
Assignee: nobody → romaxa
Attachment #496017 - Attachment is obsolete: true
Attachment #496169 - Flags: feedback?
Attachment #496017 - Flags: feedback?(roc)
+  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.
Attached patch Should be right (obsolete) — Splinter Review
Yep yesterday I was doing wrong things and was tired a bit...
Hopefully now it should be better
Attachment #496169 - Attachment is obsolete: true
Attachment #496445 - Flags: review?(roc)
Attachment #496169 - Flags: feedback?
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.
This is pretty close. It looks nice.

Once it lands we should see if it improves performance for tests that just play video.
Oh also, aFlags != INVALIDATE_NO_UPDATE_LAYER_TREE should be "if (!(aFlags & INVALIDATE_NO_UPDATE_LAYER_TREE))"
Blocks: 602080
Attached patch Fixed comments + mRoot check (obsolete) — Splinter Review
Fixed crash on first paint when mRoot is not set yet.
Checked on Qt N900

N900 (SW rendering) youtube 7FPS -> 17FPS
Attachment #496805 - Flags: review?(roc)
(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.
This should work for TYPE_CANVAS too.
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.
Attached patch Hopefully updated (obsolete) — Splinter Review
Updated, mTransactionIncomplete and disable transaction if manager is non-basic...
Attachment #496445 - Attachment is obsolete: true
Attachment #496445 - Flags: review?(roc)
Attachment #496805 - Attachment is obsolete: true
Attachment #496805 - Flags: review?(roc)
Attachment #496921 - Attachment is obsolete: true
Attachment #496939 - Flags: review?(roc)
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
Attachment #496939 - Flags: review?(roc)
Attached patch This version seems green enough (obsolete) — Splinter Review
http://hg.mozilla.org/try/rev/24c9e1cec1d7
Attachment #496939 - Attachment is obsolete: true
Attachment #497056 - Flags: review?(roc)
Attached patch Drop canvas element... (obsolete) — Splinter Review
Canvas does not work. push it without canvas
Attachment #497056 - Attachment is obsolete: true
Attachment #497083 - Flags: review?(roc)
Attachment #497056 - Flags: review?(roc)
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?
Attached patch mRoot != nsnull (obsolete) — Splinter Review
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.
Attachment #497083 - Attachment is obsolete: true
Attachment #497116 - Flags: review?(roc)
Attachment #497083 - Flags: review?(roc)
Attachment #497116 - Attachment is obsolete: true
Attachment #497117 - Flags: review?(roc)
Attachment #497116 - Flags: review?(roc)
+   * 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.
> You didn't do this.
oh sorry,
Attachment #497117 - Attachment is obsolete: true
Attachment #497118 - Flags: review?(roc)
Attachment #497117 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/34bd12eb4a9c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 618730
Depends on: 618832
Depends on: 618807
Sorry - backed this out for causing bug 618916 .
Status: RESOLVED → REOPENED
Depends on: 618916
Resolution: FIXED → ---
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.
Attached patch Bugs 618807, 618832, 618730 (obsolete) — Splinter Review
Attachment #497430 - Flags: review?(roc)
Blocks: 619056
Depends on: 618941
(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 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
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?
Also seems to have caused topcrash bug 619060.
(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?
(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.
bug 618941 is fixed by these patches.
Attached file Backtrace for empty rect (obsolete) —
Ok, damage rect is empty and has these values
86880,0,0,30540
0,30540,86880,0

And backtrace in attachment
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.
Should we track in this bug still?
Yes I think so, unless you want to file a bug that blocks this one.
Attachment #497738 - Flags: review?(roc)
tried this, but it did not help... should I call shell->rootFrame->invalidateSubTree or  something else is wrong?
Attachment #497739 - Flags: feedback?(roc)
Will keep it as separate patch
Attachment #497430 - Attachment is obsolete: true
Attachment #497740 - Flags: review?(roc)
Attachment #497430 - Flags: review?(roc)
Try calling InvalidateFrameSubtree from nsMenuPopupFrame::ShowPopup as well.
Attachment #497739 - Attachment is obsolete: true
Attachment #497744 - Flags: review?(roc)
Attachment #497739 - Flags: feedback?(roc)
Attached patch damageRect[0,0,0,0], flags:50 (obsolete) — Splinter Review
ok, for autocompl menu it is coming from FrameLayerBuilder::DrawThebesLayer, FrameLayerBuilder.cpp:1592
Attached file Visibility Break (obsolete) —
Attachment #497569 - Attachment is obsolete: true
Attachment #497746 - Attachment is obsolete: true
Attachment #497749 - Attachment is obsolete: true
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]
The original patch has problems if the transaction fails. We bail out of PaintLayer without cleaning up properly. I'll fix that.
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.
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.
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.
Attachment #497118 - Attachment is obsolete: true
Attachment #497738 - Attachment is obsolete: true
Attachment #497744 - Attachment is obsolete: true
Attachment #498709 - Flags: superreview?(jones.chris.g)
Attachment #497744 - Flags: review?(roc)
This seems better.

Oleg, please test these patches to make sure they work for you.
Attachment #497740 - Attachment is obsolete: true
Attachment #498710 - Flags: review?(tnikkel)
Attachment #497740 - Flags: review?(roc)
> 
> 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 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.
I don't think it matters; I think anything that resizes views is also going to invalidate normally.
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.
Attachment #498709 - Flags: superreview?(jones.chris.g) → superreview-
(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.
What was that case?

Would you be more comfortable if we had reflows always set the "needs layer tree update" flag?
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.
Attachment #498710 - Flags: review?(tnikkel) → review+
Attached patch Part 1 v2 (obsolete) — Splinter Review
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.
Attachment #498709 - Attachment is obsolete: true
Attachment #498984 - Flags: review?(jones.chris.g)
(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.
Attached patch Part 2 v2Splinter Review
Updated for the DoEmptyTransaction API.
Oleg, you probably should test these patches again.
Attached patch Part 1 v3Splinter Review
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.
Attachment #498984 - Attachment is obsolete: true
Attachment #498987 - Flags: review?(jones.chris.g)
Attachment #498984 - Flags: review?(jones.chris.g)
> Part 1 v3 > Part 2 v2
Tested, youtube and URL is fast
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.
Attachment #498987 - Flags: review?(jones.chris.g) → review+
(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.
Duh, right.  There might be some value in keeping these checks localized to Paint() but I don't care so much.
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
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.
Attachment #499759 - Attachment is obsolete: true
Attachment #499800 - Flags: review?(roc)
Attachment #499759 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/a07894326d5d
http://hg.mozilla.org/mozilla-central/rev/0a7d57ee6a0a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Depends on: 621762
Depends on: 622542
Depends on: 626096
Depends on: 633164
You need to log in before you can comment on or make changes to this bug.