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.

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?

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

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

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

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

(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.

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...

+ && 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.

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)

+ 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.

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

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))"

Fixed crash on first paint when mRoot is not set yet. Checked on Qt N900 N900 (SW rendering) youtube 7FPS -> 17FPS

(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.

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

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

Canvas does not work. push it without canvas

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?

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.

+ * 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,

Sorry - backed this out for causing bug 618916 .

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.

Test build available here http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/romaxa@gmail.com-8db82ab1b2d0/

(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.

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.

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

Will keep it as separate patch

Try calling InvalidateFrameSubtree from nsMenuPopupFrame::ShowPopup as well.

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

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.

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

> > 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.

Correct.

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.

(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.

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.

(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.

Updated for the DoEmptyTransaction API.

Oleg, you probably should test these patches again.

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.

> 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.

(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

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

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.

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