863477 - SurfaceCaps assertion failure in GLContext::UpdatePixelFormat() when playing Flash video

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Plugins, defect)

Description

[Chris Peterson [:cpeterson]]

When playing the following embedded YouTube video on my Nexus 4, I hit an GL assertion failure because GLContext's SurfaceCaps don't match the PixelBufferFormat. I believe this is a regression from bug 716859.

http://www.qiwireless.com/nokia-md-51w-jbl-playup-unboxing-and-demo-video/


F/MOZ_Assert( 5455): Assertion failure: caps.color == !!format.red, at gfx/gl/GLContext.cpp:877
F/libc    ( 5455): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 5473 (FP_DoPlay)


(gdb) bt
#0  0x7a652408 in mozilla::gl::GLContext::UpdatePixelFormat (this=0x78e9e400) at gfx/gl/GLContext.cpp:878
#1  0x7a65f614 in mozilla::gl::GLContext::InitOffscreen (this=0x78e9e400, size=..., caps=...) at gfx/gl/GLContext.h:1201
#2  0x7a65ffb2 in mozilla::gl::GLContextProviderEGL::CreateOffscreen (size=..., caps=..., flags=<optimized out>) at gfx/gl/GLContextProviderEGL.cpp:2361
#3  0x7a1594f2 in EnsureGLContext () at dom/plugins/base/nsNPAPIPluginInstance.cpp:87
#4  0x7a15b1ac in nsNPAPIPluginInstance::CreateSurfaceTexture (this=0x78257980) at dom/plugins/base/nsNPAPIPluginInstance.cpp:976
#5  0x7a15b246 in nsNPAPIPluginInstance::AcquireContentWindow (this=0x78257980) at dom/plugins/base/nsNPAPIPluginInstance.cpp:1001
#6  0x81ef9122 in ?? ()


(gdb) print this->mCaps
$4 = {any = false, color = false, alpha = false, bpp16 = false, depth = false, stencil = false, antialias = false, preserve = false}

(gdb) print format
$6 = {red = 8, green = 8, blue = 8, alpha = 0, depth = 0, stencil = 0, samples = 1}

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: RGBA-dummyCaps.patch

Use RGBA SurfaceCaps to avoid GLContext::UpdatePixelFormat() assertion failure when playing Flash video on Android. In bug 827407 comment 20, snorp suggested initializing dummyCaps with SurfaceCaps::ForRGBA().

Comment 2

[Kelsey Gilbert [:jgilbert]]

I think that for this case, we should be asking for SurfaceCaps::Any(), as we're not actually interested in using the offscreen buffer. We only want a GLContext to use, nothing more.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 739315 [details] [diff] [review]
RGBA-dummyCaps.patch

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

This problem should go away more properly if we use `SurfaceCaps::Any()`.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> I think that for this case, we should be asking for SurfaceCaps::Any(), as
> we're not actually interested in using the offscreen buffer. We only want a
> GLContext to use, nothing more.

With SurfaceCaps::Any(), the GLContext::UpdatePixelFormat() assertions still fail. We're comparing SurfaceCaps::Any() to the QueryPixelFormat() of the offscreen GLContext. Perhaps the assertions in UpdatePixelFormat() are themselves incorrect?

https://hg.mozilla.org/mozilla-central/annotate/d8202613aaea/gfx/gl/GLContext.cpp#l872

(gdb) bt
#0  0x7a8fcd70 in mozilla::gl::GLContext::UpdatePixelFormat (this=0x815f9c00) at central/gfx/gl/GLContext.cpp:873
#1  0x7a908fcc in mozilla::gl::GLContext::InitOffscreen (this=0x815f9c00, size=..., caps=...) at central/gfx/gl/GLContext.h:1199
#2  0x7a90983e in mozilla::gl::GLContextProviderEGL::CreateOffscreen (size=..., caps=..., flags=<optimized out>) at central/gfx/gl/GLContextProviderEGL.cpp:2008
#3  0x7a49e1b2 in EnsureGLContext () at central/dom/plugins/base/nsNPAPIPluginInstance.cpp:87
#4  0x7a49fd28 in nsNPAPIPluginInstance::CreateSurfaceTexture (this=0x7f2663e0) at central/dom/plugins/base/nsNPAPIPluginInstance.cpp:976
#5  0x7a49fdc2 in nsNPAPIPluginInstance::AcquireContentWindow (this=0x7f2663e0) at central/dom/plugins/base/nsNPAPIPluginInstance.cpp:1001
#6  0x82639122 in ?? ()

(gdb) print format
$3 = {red = 8, green = 8, blue = 8, alpha = 0, depth = 0, stencil = 0, samples = 1}

(gdb) print mCaps
$4 = {any = true, color = false, alpha = false, bpp16 = false, depth = false, stencil = false, antialias = false, preserve = false}

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Use SurfaceCaps::Any, and make offscreen GLContexts DetermineCaps().

I think this is what we want. Does this work?

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 741114 [details] [diff] [review]
Use SurfaceCaps::Any, and make offscreen GLContexts DetermineCaps().

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

I still hit MOZ_ASSERT(caps.color == !!format.red) because format.red == 8, but gdb can't see much more because I'm debugging an optimized debug build. I'll need to debug this in an unoptimized build for more info.

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Chris Peterson (:cpeterson) from comment #6)
> Comment on attachment 741114 [details] [diff] [review]
> Use SurfaceCaps::Any, and make offscreen GLContexts DetermineCaps().
> 
> Review of attachment 741114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still hit MOZ_ASSERT(caps.color == !!format.red) because format.red == 8,
> but gdb can't see much more because I'm debugging an optimized debug build.
> I'll need to debug this in an unoptimized build for more info.

This should not be possible with the patch. DetermineCaps and UpdatePixelFormat both use QueryPixelFormat. Please step through DetermineCaps to assure that the `format` there matches the `format` we later check ourselves against in UpdatePixelFormat.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 741114 [details] [diff] [review]
Use SurfaceCaps::Any, and make offscreen GLContexts DetermineCaps().

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

LGTM. Your patch works for me this morning. I must have been debugging the wrong build yesterday.

Comment 9

[Chris Peterson [:cpeterson]]

Try test run:
https://tbpl.mozilla.org/?tree=Try&rev=45c61074daa7

Comment 10

[Chris Peterson [:cpeterson]]

Jeff, I can land your patch tomorrow. The try builds look good.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to Chris Peterson (:cpeterson) from comment #10)
> Jeff, I can land your patch tomorrow. The try builds look good.

That will work fine.

Comment 12

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5badac802f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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