783043 - Video's ImageContainer uses a wrong BasicImageFactory with layers acceleration on Linux

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

This bug does not happen when using OMTC + layers acceleration, only with layers acceleration.

On this page: http://people.mozilla.org/~bgirard/Video.html

Right after the page is loaded, we apparently render it into a canvas (for the thumbnails?) which will put us in the stacktrace below.

The sad part is that inside this code path we use a BasicLayerManager, and set the ImageContainer's ImageFactory to the BasicManager's BasicImageFactory (see BasicImageLayer::GetAndPaintCurrentImage).

After this the ImageContainer keeps the factory and everytime we have the ImageContainer create a new image, it uses the BasicImageFactory which creates a BasicPlanarYCbCrImage instead of a PlanarYCbCrImage.

As a result the YUV->RGB conversion is done right away when calling SetData on the image, and done another time in ImageLayerOGL::RenderLayer in the shader.

It does not seem to happen on mac.

--------------------------------------------------------------

#1 in mozilla::layers::BasicLayerManager::GetImageFactory at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicImages.cpp:16
#2 in mozilla::layers::BasicImageLayer::GetAndPaintCurrentImage at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicImageLayer.cpp:8
#3 in mozilla::layers::BasicImageLayer::Paint at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicImageLayer.cpp:7
#4 in mozilla::layers::BasicLayerManager::PaintLayer at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:83
#5 in mozilla::layers::BasicLayerManager::EndTransactionInternal at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:46
#6 in mozilla::layers::BasicLayerManager::EndTransaction at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:39
#7 in mozilla::PaintInactiveLayer at /home/nico/dev/mozilla/mozilla-central/layout/base/FrameLayerBuilder.cpp:167
#8 in mozilla::FrameLayerBuilder::DrawThebesLayer at /home/nico/dev/mozilla/mozilla-central/layout/base/FrameLayerBuilder.cpp:282
#9 in mozilla::layers::BasicThebesLayer::PaintThebes at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicThebesLayer.cpp:14
#10 in mozilla::layers::BasicLayerManager::PaintLayer at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:82
#11 in mozilla::layers::BasicLayerManager::PaintLayer at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:84
#12 in mozilla::layers::BasicLayerManager::EndTransactionInternal at /home/nico/dev/mozilla/mozilla-central/gfx/layers/basic/BasicLayerManager.cpp:46
#13 in mozilla::layers::BasicLayerManager::EndTransaction at /home/nico/dev/mozilla/mozilla-central/gfx/laye
#14 in nsDisplayList::PaintForFrame at /home/nico/dev/mozilla/mozilla-central/layout/base/nsDisplayList.cpp:104
#15 in nsDisplayList::PaintRoot at /home/nico/dev/mozilla/mozilla-central/layout/base/nsDisplayList.cpp:94
#16 in nsLayoutUtils::PaintFrame at /home/nico/dev/mozilla/mozilla-central/layout/base/nsLayoutUtils.cpp:186
#17 in PresShell::RenderDocument at /home/nico/dev/mozilla/mozilla-central/layout/base/nsPresShell.cpp:441
#18 in nsCanvasRenderingContext2D::DrawWindow at /home/nico/dev/mozilla/mozilla-central/content/canvas/src/nsCanvasRenderingContext2D.cpp:372
#19 in nsIDOMCanvasRenderingContext2D_DrawWindow at /home/nico/dev/mozilla/obj-Linux64dbg/js/xpconnect/src/dom_quickstubs.cpp:280
#20 in js::CallJSNative at /home/nico/dev/mozilla/mozilla-central/js/src/jscntxtinlines.h:38
#21 in js::InvokeKernel at /home/nico/dev/mozilla/mozilla-central/js/src/jsinterp.cpp:35
#22 in js::Invoke at /home/nico/dev/mozilla/mozilla-central/js/src/jsinterp.h:11
#23 in js::Invoke at /home/nico/dev/mozilla/mozilla-central/js/src/jsinterp.cpp:39
#24 in js::IndirectProxyHandler::call at /home/nico/dev/mozilla/mozilla-central/js/src/jsproxy.cpp:47
#25 in js::DirectWrapper::call at /home/nico/dev/mozilla/mozilla-central/js/src/jswrapper.cpp:31
#26 in js::CrossCompartmentWrapper::call at /home/nico/dev/mozilla/mozilla-central/js/src/jswrapper.cpp:73
#27 in js::Proxy::call at /home/nico/dev/mozilla/mozilla-central/js/src/jsproxy.cpp:131
#28 in proxy_Call at /home/nico/dev/mozilla/mozilla-central/js/src/jsproxy.cpp:185

Comment 1

[Nicolas Silva [:nical]]

I am not sure what the source of the problem actually is:
 - Are we using the wrong code path (rendering the page through a basic layer manager)?
 - Is it bad that BasicImageLayer::GetAndPaintCurrentImage changes the ImageFactory to a BasicImageFactory without resetting it to it's initial value afterward? (this looks bad to me)

I tried to call mContainer->SetImageFactory(nullptr) before returning from GetAndPaintCurrentImage, which sets the ImageFactory to new ImageFactory() internally, it fixes the problem with layer acceleration, and it looks like it does not break non-accelerated layers (I just tried with a webm video and a few websites, though), but I don't like this solution because I don't know if, in some cases, before GetAndPaintCurrentImage the factory may be already a BasicImageFactory, which would not really reset the factory to its original value.

Thoughts?

Comment 2

[Nicolas Silva [:nical]]

It seems to me that we don't need this line : 
http://dxr.mozilla.org/mozilla-central/gfx/layers/basic/BasicImageLayer.cpp.html?string=BasicImageLayer.cpp#l84

mContainer->SetImageFactory(mManager->IsCompositingCheap() ? nullptr : BasicManager()->GetImageFactory());

Can anyone convince me that we do?

Changing the ImageFactory during the rendering of a screenshot is a bad thing because it messes the pipeline for actual rendering of the page (because of this line we do an extra copy + yuc->rgb conversion with linux and accelerated layers.

We could reset the image factory at the end of the function, but in the end BasicPlanarYCbCrImage only provides with an early color conversion, and we would also have the conversion rather soon with a PlanarYCbCrImage with line 87.

The risk would be that we call GetAsSurface() several times per frame on the images produced by the image factory (since PlanarYCbCrImage does the conversion each time GetAsSurface is called), but I don't see why we would do this often.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Don't mess up the video pipeline by changing the imagefactory

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 665514 [details] [diff] [review]
Don't mess up the video pipeline by changing the imagefactory

I'm not familiar with the image factory stuff. Bas?

Comment 5

[Nicolas Silva [:nical]]

Attachment: Don't mess up the video pipeline by changing the imagefactory

The previous patch had some a few changes that i did not mean to export.

Basically the problem here is that we have a page that is usually rendered through a GL layer manager, but when we render the thumbnail we render it using a basic layer manager, both layer managers pointing to the same ImageContainer. So when one of these layer managers changes the state of the container, this change is kept when the other one uses it. Obviously this is bad, in our case here the basic image factory stays in the ImageContainer forever after we have rendered the thumbnail, and leads to some slow code paths for every video frames.

my conclusion: anything that changes states in the resources that outlive the render pass should either be removed or make sure that the states are reverted to what they were before, to avoid messing up a rendering pipeline like we do here.
In this particular case I believe that BasicPlanarYCbCrImage is only preferable over PlanarYCbCrImage if we invoke GetAsSurface several times per image, which is odd because we should only do this when we want to read the pixels, which should (most of the time) only happen when we will composite the image.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 680839 [details] [diff] [review]
Don't mess up the video pipeline by changing the imagefactory

Review of attachment 680839 [details] [diff] [review]:
-----------------------------------------------------------------

this is kind of outdated now i guess

Comment 7

[Clément Geiger [:cgg]]

Attachment: Restore original image factory after paint operations

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8443908 [details] [diff] [review]
Restore original image factory after paint operations

Review of attachment 8443908 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, this patch just needs the modification below to get r+ed.

::: gfx/layers/basic/BasicImageLayer.cpp
@@ +74,4 @@
>      return;
>    }
>  
> +  ImageFactory* originalIF = mContainer->GetImageFactory();

ImageFactory is a reference-counted objetc. This means that during SetImageFactory, the reference count of the ImageFactory you just referenced in originalIF could reach 0, which would delete the object and let us with a dangling originalIF pointer.

If you replace "ImageFactory* originalIF" by "nsRefPtr<ImageFactory> originalIF" the patch will be correct since the nsRefPtr will make sure the ref count doesn't reach zero in this scope.

@@ +105,4 @@
>      return;
>    }
>  
> +  ImageFactory* originalIF = mContainer->GetImageFactory();

Same issue here.

Comment 9

[Clément Geiger [:cgg]]

Attachment: Restore original image factory after paint operations

Comment 10

[Nicolas Silva [:nical]]

try push:  https://tbpl.mozilla.org/?tree=Try&rev=ce3cd95c1bd0

Comment 11

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29bc4fa125d9

Comment 12

[Nicolas Silva [:nical]]

The patch had formatting issues so I backed it out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2563b5f12f5 

Then fixed it and relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d171f24b21cf

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d171f24b21cf