In order to allow layers to manipulate TexClients, we need to switch from subverting the layers mechanisms via SurfaceStream to embracing them with a SharedSurfaceTextureClient. In the short term, this is just a simplification of what we already do, but it will allow for doing more sophisticated things with layers transactions in the long run. (As well as just being a great simplification)

r=kamidphish from bug 1052234

r=kamidphish in bug 1052234

Comment on attachment 8488170 [details] [diff] [review] patch 3: Add TexClient for ShSurf Review of attachment 8488170 [details] [diff] [review]: ----------------------------------------------------------------- I've looked over the GL changes. Matt can you cast your expert eye over the CanvasClient, TextureClient/Host, and Layer changes? ::: gfx/2d/HelpersCairo.h @@ +140,5 @@ > case SurfaceFormat::R5G6B5: > return CAIRO_FORMAT_RGB16_565; > default: > gfxWarning() << "Unknown image format"; > + MOZ_ASSERT(false, "Unknown image format"); Good. ::: gfx/gl/GLContext.h @@ +1516,5 @@ > BEFORE_GL_CALL; > mSymbols.fReadBuffer(mode); > AFTER_GL_CALL; > } > + To the god of WS?

Comment on attachment 8488170 [details] [diff] [review] patch 3: Add TexClient for ShSurf Review of attachment 8488170 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +1706,2 @@ > > + return nullptr; Might as well just remove the callers of this then and kill it? ::: gfx/layers/client/CanvasClient.cpp @@ +216,5 @@ > bool bufferCreated = false; > if (!mBuffer) { > + // We need to dealloc in the client. > + TextureFlags flags = GetTextureFlags() | > + TextureFlags::DEALLOCATE_CLIENT; CreateCanvasClient should have set DEALLOCATE_CLIENT already. ::: gfx/layers/client/CanvasClient.h @@ +44,5 @@ > */ > enum CanvasClientType { > CanvasClientSurface, > CanvasClientGLContext, > + CanvasClientTypeShSurf, Once we convert to creating the specific TextureClients we can probably get rid of the other two types. ::: gfx/layers/client/TextureClient.h @@ +681,5 @@ > > +/** > + * A TextureClient implementation to share SharedSurfaces. > + */ > +class ShSurfTexClient : public TextureClient SharedSurfaceTextureClient ::: gfx/layers/composite/TextureHost.h @@ +653,5 @@ > > +/** > + * A TextureHost for SharedSurfaces > + */ > +class ShSurfTexHost : public TextureHost SharedSurfaceTextureHost

(In reply to Dan Glastonbury :djg :kamidphish from comment #4) > Comment on attachment 8488170 [details] [diff] [review] > patch 3: Add TexClient for ShSurf > > Review of attachment 8488170 [details] [diff] [review]: > ----------------------------------------------------------------- > > I've looked over the GL changes. Matt can you cast your expert eye over the > CanvasClient, TextureClient/Host, and Layer changes? > > ::: gfx/2d/HelpersCairo.h > @@ +140,5 @@ > > case SurfaceFormat::R5G6B5: > > return CAIRO_FORMAT_RGB16_565; > > default: > > gfxWarning() << "Unknown image format"; > > + MOZ_ASSERT(false, "Unknown image format"); > > Good. > > ::: gfx/gl/GLContext.h > @@ +1516,5 @@ > > BEFORE_GL_CALL; > > mSymbols.fReadBuffer(mode); > > AFTER_GL_CALL; > > } > > + > > To the god of WS? Not today!

(In reply to Matt Woodrow (:mattwoodrow) from comment #5) > Comment on attachment 8488170 [details] [diff] [review] > patch 3: Add TexClient for ShSurf > > Review of attachment 8488170 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/GLContext.cpp > @@ +1706,2 @@ > > > > + return nullptr; > > Might as well just remove the callers of this then and kill it? Right, done. > > ::: gfx/layers/client/CanvasClient.cpp > @@ +216,5 @@ > > bool bufferCreated = false; > > if (!mBuffer) { > > + // We need to dealloc in the client. > > + TextureFlags flags = GetTextureFlags() | > > + TextureFlags::DEALLOCATE_CLIENT; > > CreateCanvasClient should have set DEALLOCATE_CLIENT already. We actually don't need this, so I'll remove it. > > ::: gfx/layers/client/CanvasClient.h > @@ +44,5 @@ > > */ > > enum CanvasClientType { > > CanvasClientSurface, > > CanvasClientGLContext, > > + CanvasClientTypeShSurf, > > Once we convert to creating the specific TextureClients we can probably get > rid of the other two types. Yep! > > ::: gfx/layers/client/TextureClient.h > @@ +681,5 @@ > > > > +/** > > + * A TextureClient implementation to share SharedSurfaces. > > + */ > > +class ShSurfTexClient : public TextureClient > > SharedSurfaceTextureClient So many letters! :P > > ::: gfx/layers/composite/TextureHost.h > @@ +653,5 @@ > > > > +/** > > + * A TextureHost for SharedSurfaces > > + */ > > +class ShSurfTexHost : public TextureHost > > SharedSurfaceTextureHost Yeah, ok.

Any ETA on landing now that everything is reviewed?

It is failing on try and I don't have access to the android/b2gemu setups to test it myself. (I'm still at the Khronos F2F) These are the two most recent Try runs: https://tbpl.mozilla.org/?tree=Try&rev=8a67d53ea1e7 https://tbpl.mozilla.org/?tree=Try&rev=e705fef5899b

New run, forgot to restrict it to b2gemu. https://tbpl.mozilla.org/?tree=Try&rev=684951085fba

Perhaps, I'm misunderstanding but does this patch set move the WaitSync call from compositor side to client side? If so, won't that cause performance regressions for b2g etc?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > Perhaps, I'm misunderstanding but does this patch set move the WaitSync call > from compositor side to client side? If so, won't that cause performance > regressions for b2g etc? It does not do that.

(In reply to Jeff Gilbert [:jgilbert] from comment #12) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > > Perhaps, I'm misunderstanding but does this patch set move the WaitSync call > > from compositor side to client side? If so, won't that cause performance > > regressions for b2g etc? > > It does not do that. Where is the new call to WaitSync()?

(In reply to Jeff Muizelaar [:jrmuizel] from comment #13) > (In reply to Jeff Gilbert [:jgilbert] from comment #12) > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > > > Perhaps, I'm misunderstanding but does this patch set move the WaitSync call > > > from compositor side to client side? If so, won't that cause performance > > > regressions for b2g etc? > > > > It does not do that. > > Where is the new call to WaitSync()? In SharedSurfaceTextureHost::EnsureTexSource().

r=kamidphish

r=kamidphish

r=kamidphish,mattwoodrow

I got frustrated by canvas2d issues, and removing SurfaceStream simplifies things, so I did that.

Try seems green! https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=262d999e989c

Comment on attachment 8500788 [details] [diff] [review] patch 3.5: Fixes to patch 3 Review of attachment 8500788 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContext.cpp @@ +1416,4 @@ > > gl->MakeCurrent(); > + > + auto screen = gl->Screen(); Please don't use auto unless the type is otherwise immediately obvious the reader. ::: gfx/layers/opengl/GrallocTextureClient.cpp @@ +347,5 @@ > return 0; > } > > +/*static*/ TemporaryRef<TextureClient> > +GrallocTextureClientOGL::FromShSurf(gl::SharedSurface* abstractSurf, FromSharedSurface

Comment on attachment 8500788 [details] [diff] [review] patch 3.5: Fixes to patch 3 Review of attachment 8500788 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLScreenBuffer.cpp @@ +43,5 @@ > UniquePtr<SurfaceFactory> factory; > > #ifdef MOZ_WIDGET_GONK > /* On B2G, we want a Gralloc factory, and we want one right at the start */ > + auto allocator = caps.surfaceAllocator; auto is the new var? ::: gfx/gl/GLUploadHelpers.cpp @@ +461,5 @@ > + case SurfaceFormat::R8G8B8A8: > + if (gl->GetPreferredARGB32Format() == LOCAL_GL_BGRA) { > + format = LOCAL_GL_BGRA; > + type = LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV; > + surfaceFormat = SurfaceFormat::B8G8R8A8; Is this saying to swap the BGRA order to get RGBA? ::: gfx/layers/opengl/GrallocTextureClient.cpp @@ +112,5 @@ > android::sp<Fence> fence = mReleaseFenceHandle.mFence; > #if ANDROID_VERSION == 17 > fence->waitForever(1000, "GrallocTextureClientOGL::Lock"); > // 1000 is what Android uses. It is warning timeout ms. > + // This timeous is removed since ANDROID_VERSION 18. timeout?

Comment on attachment 8500789 [details] [diff] [review] patch 4: Remove SurfaceStream Review of attachment 8500789 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +4833,5 @@ > + data.mSize = nsIntSize(mWidth, mHeight); > + data.mHasAlpha = !mOpaque; > + > + SkiaGLGlue* glue = gfxPlatform::GetPlatform()->GetSkiaGLGlue(); > + GLuint skiaGLTex = (GLuint)(uintptr_t)mTarget->GetNativeSurface(NativeSurfaceType::OPENGL_TEXTURE); Wat?! double cast.

Comment on attachment 8500790 [details] [diff] [review] patch 4.5: Fixes for patch 4 Review of attachment 8500790 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +4795,2 @@ > SkiaGLGlue* glue = gfxPlatform::GetPlatform()->GetSkiaGLGlue(); > + GLuint skiaGLTex = (GLuint)(uintptr_t)mTarget->GetNativeSurface(NativeSurfaceType::OPENGL_TEXTURE); Double cast? ::: gfx/2d/DrawTargetSkia.cpp @@ +704,5 @@ > TempBitmap bitmap = GetBitmapForSurface(aSurface); > > // This is a fast path that is disabled for now to mimimize risk > if (false && !bitmap.mBitmap.getTexture() && mCanvas->imageInfo() == bitmap.mBitmap.info()) { > + SkBitmap bm(bitmap.mBitmap); Indent. ::: gfx/gl/SharedSurfaceGralloc.cpp @@ +67,5 @@ > gfxContentType type = hasAlpha ? gfxContentType::COLOR_ALPHA > : gfxContentType::COLOR; > > + auto platform = gfxPlatform::GetPlatform(); > + gfxImageFormat format = platform->OptimalFormatForContent(type); why is platform auto, yet format is gfxImageFormat? ::: gfx/layers/CopyableCanvasLayer.cpp @@ +95,5 @@ > } > return; > } > > + if (mDrawTarget) { Don't need { }.

Comment on attachment 8500789 [details] [diff] [review] patch 4: Remove SurfaceStream Review of attachment 8500789 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SurfaceTypes.cpp @@ +46,5 @@ > bpp16 = false; > depth = false; > stencil = false; > antialias = false; > + premultAlpha = true; Why this change? ::: gfx/layers/CopyableCanvasLayer.cpp @@ +109,5 @@ > + if (mGLFrontbuffer) { > + frontbuffer = mGLFrontbuffer.get(); > + } else { > + auto screen = mGLContext->Screen(); > + auto front = screen->Front(); no auto please ::: gfx/layers/client/CanvasClient.cpp @@ +140,5 @@ > aFormat, aSize, backend, > mTextureInfo.mTextureFlags | aFlags); > #endif > } > +/* Remove this code instead of commenting it out.

Comment on attachment 8500790 [details] [diff] [review] patch 4.5: Fixes for patch 4 Review of attachment 8500790 [details] [diff] [review]: ----------------------------------------------------------------- It's nice seeing indent and whitespace being cleaned up, but please put those changes into a separate patch from functional changes. The CopyableCanvasLayer change worries me, will wait to hear back about that. ::: gfx/2d/DrawTargetSkia.cpp @@ +715,5 @@ > + // adjust pixels for the source offset > + pixels += aSourceRect.x + aSourceRect.y*bm.rowBytes(); > + mCanvas->writePixels(info, pixels, bm.rowBytes(), aDestination.x, aDestination.y); > + return; > + } Indent all the way ::: gfx/gl/SharedSurfaceGralloc.cpp @@ +72,3 @@ > > + typedef GrallocTextureClientOGL ptrT; > + RefPtr<ptrT> grallocTC = new ptrT(allocator, I'm not convinced these changes made anything more readable :) ::: gfx/layers/CopyableCanvasLayer.cpp @@ -81,5 @@ > CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget) > { > - if (!IsDirty()) > - return; > - Painted(); Why was this removed? We don't want to readback from the GL texture every frame if it hasn't changed. I see that ClientCanvasLayer checks this itself, but BasicCanvasLayer will be regressed.

(In reply to Matt Woodrow (:mattwoodrow) from comment #27) > Comment on attachment 8500790 [details] [diff] [review] > patch 4.5: Fixes for patch 4 > > Review of attachment 8500790 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's nice seeing indent and whitespace being cleaned up, but please put > those changes into a separate patch from functional changes. > > The CopyableCanvasLayer change worries me, will wait to hear back about that. > > ::: gfx/2d/DrawTargetSkia.cpp > @@ +715,5 @@ > > + // adjust pixels for the source offset > > + pixels += aSourceRect.x + aSourceRect.y*bm.rowBytes(); > > + mCanvas->writePixels(info, pixels, bm.rowBytes(), aDestination.x, aDestination.y); > > + return; > > + } > > Indent all the way Oops, ok. > > ::: gfx/gl/SharedSurfaceGralloc.cpp > @@ +72,3 @@ > > > > + typedef GrallocTextureClientOGL ptrT; > > + RefPtr<ptrT> grallocTC = new ptrT(allocator, > > I'm not convinced these changes made anything more readable :) Fair. I can cut the decl and definition to different lines and still keep it short. > > ::: gfx/layers/CopyableCanvasLayer.cpp > @@ -81,5 @@ > > CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget) > > { > > - if (!IsDirty()) > > - return; > > - Painted(); > > Why was this removed? We don't want to readback from the GL texture every > frame if it hasn't changed. I see that ClientCanvasLayer checks this itself, > but BasicCanvasLayer will be regressed. Ok, we need to change BasicCanvasLayer to manage its own dirtyness. This bug was a lot of trouble to track down. Dirtying should always happen at the same layer. The issue here was that we mark the layer clean, and then after marking it clean, call UpdateTarget. Since the layer's clean, UpdateTarget exits early, and does no work.

Comment on attachment 8500789 [details] [diff] [review] patch 4: Remove SurfaceStream Snorp is on PTO, and two sets of eyes is probably enough.

Comment on attachment 8500790 [details] [diff] [review] patch 4.5: Fixes for patch 4 Snorp is on PTO, and two sets of eyes is probably enough.

(In reply to Dan Glastonbury :djg :kamidphish from comment #23) > Comment on attachment 8500788 [details] [diff] [review] > patch 3.5: Fixes to patch 3 > > Review of attachment 8500788 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/GLScreenBuffer.cpp > @@ +43,5 @@ > > UniquePtr<SurfaceFactory> factory; > > > > #ifdef MOZ_WIDGET_GONK > > /* On B2G, we want a Gralloc factory, and we want one right at the start */ > > + auto allocator = caps.surfaceAllocator; > > auto is the new var? Yep. I've gone off the deep end for them. We still have static types, but I just don't have to look up the exact namespaced name when I can have the compiler handle that for me. I added the type back in for this case, and can add it back for any others. > > ::: gfx/gl/GLUploadHelpers.cpp > @@ +461,5 @@ > > + case SurfaceFormat::R8G8B8A8: > > + if (gl->GetPreferredARGB32Format() == LOCAL_GL_BGRA) { > > + format = LOCAL_GL_BGRA; > > + type = LOCAL_GL_UNSIGNED_INT_8_8_8_8_REV; > > + surfaceFormat = SurfaceFormat::B8G8R8A8; > > Is this saying to swap the BGRA order to get RGBA? Yep. We're uploading RGBA data to GL as BGRA, and then just remembering that we need to sample from B to get R. I added a comment saying this. > > ::: gfx/layers/opengl/GrallocTextureClient.cpp > @@ +112,5 @@ > > android::sp<Fence> fence = mReleaseFenceHandle.mFence; > > #if ANDROID_VERSION == 17 > > fence->waitForever(1000, "GrallocTextureClientOGL::Lock"); > > // 1000 is what Android uses. It is warning timeout ms. > > + // This timeous is removed since ANDROID_VERSION 18. > > timeout? Yep, fixed.

(In reply to Dan Glastonbury :djg :kamidphish from comment #24) > Comment on attachment 8500789 [details] [diff] [review] > patch 4: Remove SurfaceStream > > Review of attachment 8500789 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +4833,5 @@ > > + data.mSize = nsIntSize(mWidth, mHeight); > > + data.mHasAlpha = !mOpaque; > > + > > + SkiaGLGlue* glue = gfxPlatform::GetPlatform()->GetSkiaGLGlue(); > > + GLuint skiaGLTex = (GLuint)(uintptr_t)mTarget->GetNativeSurface(NativeSurfaceType::OPENGL_TEXTURE); > > Wat?! double cast. Yep, the existing code already does it. In some ways it makes sense, first stepping from a void* to a uintptr_t, but then truncating to a GLuint.

(In reply to Dan Glastonbury :djg :kamidphish from comment #25) > Comment on attachment 8500790 [details] [diff] [review] > patch 4.5: Fixes for patch 4 > > Review of attachment 8500790 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: gfx/2d/DrawTargetSkia.cpp > @@ +704,5 @@ > > TempBitmap bitmap = GetBitmapForSurface(aSurface); > > > > // This is a fast path that is disabled for now to mimimize risk > > if (false && !bitmap.mBitmap.getTexture() && mCanvas->imageInfo() == bitmap.mBitmap.info()) { > > + SkBitmap bm(bitmap.mBitmap); > > Indent. Oops. > > ::: gfx/gl/SharedSurfaceGralloc.cpp > @@ +67,5 @@ > > gfxContentType type = hasAlpha ? gfxContentType::COLOR_ALPHA > > : gfxContentType::COLOR; > > > > + auto platform = gfxPlatform::GetPlatform(); > > + gfxImageFormat format = platform->OptimalFormatForContent(type); > > why is platform auto, yet format is gfxImageFormat? I think it's relevant which of our many format types `format` is here, but it's not really relevant what type `platform` has, only that it has this function call. I'm primarily defining `platform` separately to help with line length. > > ::: gfx/layers/CopyableCanvasLayer.cpp > @@ +95,5 @@ > > } > > return; > > } > > > > + if (mDrawTarget) { > > Don't need { }. gfx/layers likes to use {} everywhere.

(In reply to Matt Woodrow (:mattwoodrow) from comment #26) > Comment on attachment 8500789 [details] [diff] [review] > patch 4: Remove SurfaceStream > > Review of attachment 8500789 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/SurfaceTypes.cpp > @@ +46,5 @@ > > bpp16 = false; > > depth = false; > > stencil = false; > > antialias = false; > > + premultAlpha = true; > > Why this change? It's pretty arbitrary, but since I understand the canvas2d paths less, and canvas2d is always premult, I figure this is safer. > > ::: gfx/layers/CopyableCanvasLayer.cpp > @@ +109,5 @@ > > + if (mGLFrontbuffer) { > > + frontbuffer = mGLFrontbuffer.get(); > > + } else { > > + auto screen = mGLContext->Screen(); > > + auto front = screen->Front(); > > no auto please Ok. > > ::: gfx/layers/client/CanvasClient.cpp > @@ +140,5 @@ > > aFormat, aSize, backend, > > mTextureInfo.mTextureFlags | aFlags); > > #endif > > } > > +/* > > Remove this code instead of commenting it out. Thanks for reminding me.

patch 5: Review fixes

Comment on attachment 8500790 [details] [diff] [review] patch 4.5: Fixes for patch 4 Review of attachment 8500790 [details] [diff] [review]: ----------------------------------------------------------------- It's nice seeing indent and whitespace being cleaned up, but please put those changes into a separate patch from functional changes. The CopyableCanvasLayer change worries me, will wait to hear back about that. ::: gfx/2d/DrawTargetSkia.cpp @@ +715,5 @@ > + // adjust pixels for the source offset > + pixels += aSourceRect.x + aSourceRect.y*bm.rowBytes(); > + mCanvas->writePixels(info, pixels, bm.rowBytes(), aDestination.x, aDestination.y); > + return; > + } Indent all the way ::: gfx/gl/SharedSurfaceGralloc.cpp @@ +72,3 @@ > > + typedef GrallocTextureClientOGL ptrT; > + RefPtr<ptrT> grallocTC = new ptrT(allocator, I'm not convinced these changes made anything more readable :) ::: gfx/layers/CopyableCanvasLayer.cpp @@ -81,5 @@ > CopyableCanvasLayer::UpdateTarget(DrawTarget* aDestTarget) > { > - if (!IsDirty()) > - return; > - Painted(); Why was this removed? We don't want to readback from the GL texture every frame if it hasn't changed. I see that ClientCanvasLayer checks this itself, but BasicCanvasLayer will be regressed.

Comment on attachment 8502093 [details] [diff] [review] patch 4.6: 0001-Handle-dirtying-in-BasicCanvasLayer.patch Review of attachment 8502093 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the transaction callbacks moved. ::: gfx/layers/basic/BasicCanvasLayer.cpp @@ +32,5 @@ > + Painted(); > + > + FirePreTransactionCallback(); > + UpdateTarget(); > + FireDidTransactionCallback(); I think we want to fire the pre/post transaction callbacks unconditionally, not inside the IsDirty scope.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=881fbd15be06 Appears to be green.

(In reply to Jeff Gilbert [:jgilbert] from comment #33) > (In reply to Dan Glastonbury :djg :kamidphish from comment #25) > > Don't need { }. > gfx/layers likes to use {} everywhere. WWBED? :-)

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0db54f1033 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/642bfa833485 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a957533009df remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/66f871561005 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/72ce4c2ccdcb remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/49531b0af9b8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bc505ee945 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/216915390f9b

Bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/57a53c8b472e

https://hg.mozilla.org/mozilla-central/rev/ec0db54f1033 https://hg.mozilla.org/mozilla-central/rev/642bfa833485 https://hg.mozilla.org/mozilla-central/rev/a957533009df https://hg.mozilla.org/mozilla-central/rev/66f871561005 https://hg.mozilla.org/mozilla-central/rev/72ce4c2ccdcb https://hg.mozilla.org/mozilla-central/rev/49531b0af9b8 https://hg.mozilla.org/mozilla-central/rev/a3bc505ee945 https://hg.mozilla.org/mozilla-central/rev/216915390f9b https://hg.mozilla.org/mozilla-central/rev/57a53c8b472e

Patch 1 seems to have introduced this crash: [@ _moz_cairo_surface_destroy ] bp-bb81c8a8-708b-4a4e-bac3-1ef702141011 A brief description of the problem is here: http://forums.mozillazine.org/viewtopic.php?p=13814603#p13814603 When I have more time I can look into opening a new bug with an STR, if still necessary by then.

Could this patch be responsible for crash of OpenGL ATI driver? https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A35.0a1&version=Firefox%3A36.0a1&platform=win&range_value=1&range_unit=weeks&date=10%2F14%2F2014+11%3A00%3A00&query_search=signature&query_type=contains&query=atioglx&reason=&release_channels=&build_id=&process_type=any&hang_type=crash

The crashkill team believes that this patch is likely the cause of several new nightly crashes: bug 1081790 and a spike in the rate of the _moz_cairo_surface_destroy signature tracked by bug 1071079. We believe that this should be backed out of Aurora/35 before updates are enabled, and unless an obvious fix is in place, we also recommend backing this out of nightly/36.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #48) > The crashkill team believes that this patch is likely the cause of several > new nightly crashes: bug 1081790 and a spike in the rate of the > _moz_cairo_surface_destroy signature tracked by bug 1071079. We believe that > this should be backed out of Aurora/35 before updates are enabled, and > unless an obvious fix is in place, we also recommend backing this out of > nightly/36. No fix, so skipping Aurora uplift is probably a good idea. I need STRs or ideas for the issues before removing it from nightly. Without a better idea of the issues, I can't hope to fix them. They did not come up in testing.

To be clear, this already got uplifted to Aurora (automatically on merge day) and so the request is to back it out. If you don't know what's causing the nightly issues, dmajor or I can help you navigate the crash-stats, but we should probably back it out in the meantime.

(In reply to IU from comment #46) > Patch 1 seems to have introduced this crash: [@ _moz_cairo_surface_destroy ] > bp-bb81c8a8-708b-4a4e-bac3-1ef702141011 > > A brief description of the problem is here: > http://forums.mozillazine.org/viewtopic.php?p=13814603#p13814603 > > When I have more time I can look into opening a new bug with an STR, if > still necessary by then. Patch 1 doesn't change anything, though. Did you mean a different patch, or just the patches in general?

(In reply to Jeff Gilbert [:jgilbert] from comment #51) > Patch 1 doesn't change anything, though. Did you mean a different patch, or > just the patches in general? It was a quick look for a candidate -- not that I understand the code. :-) Also, after the crashes occurred, my GPU stopped being able to display videos and I had to reboot my computer. If I'm still able to reproduce it, is there some type of output I should collect that might help?

(In reply to IU from comment #52) > (In reply to Jeff Gilbert [:jgilbert] from comment #51) > > Patch 1 doesn't change anything, though. Did you mean a different patch, or > > just the patches in general? > > It was a quick look for a candidate -- not that I understand the code. :-) > > Also, after the crashes occurred, my GPU stopped being able to display > videos and I had to reboot my computer. > > If I'm still able to reproduce it, is there some type of output I should > collect that might help? Yes, please! An about:support would be helpful.

I used the Oct 11 nightly build, since that's the build with which I first experience the problem. I ran Firefox with WinDbg and visited the youtube video I originally experience the problem with. Once I notice the following appearing in the log, over and over again, and the WinDbg log window flashing and glitching, I felt that was enough (I did not wait for the crash): Failed to create GLContext for SkiaGL! Can't find symbol 'wglCreatePbufferARB'. Can't find symbol 'wglDestroyPbufferARB'. Can't find symbol 'wglGetPbufferDCARB'. Can't find symbol 'wglBindTexImageARB'. Can't find symbol 'wglReleaseTexImageARB'. Can't find symbol 'wglChoosePixelFormatARB'. Can't find symbol 'wglGetPixelFormatAttribivARB'. Can't find symbol 'wglGetExtensionsStringARB'. Failed to create GLContext for SkiaGL! Failed to create GLContext for SkiaGL! Failed to create GLContext for SkiaGL! Failed to create GLContext for SkiaGL! Failed to create GLContext for SkiaGL! I have the attached the WinDbg log ----------------------------------- Youtube video link: https://www.youtube.com/watch?v=SlDdjtLincE&hd=1 Here is my about:support information: -------------------------------------- Application Basics ------------------ Name: Firefox Version: 35.0a1 User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 Multiprocess Windows: 0/4 Crash Reports for the Last 3 Days --------------------------------- All Crash Reports Extensions ---------- Name: Adblock Plus Pop-up Addon Version: 0.9.2 Enabled: true ID: adblockpopups@jessehakanen.net Name: Add Bookmark Here ² Version: 29.0.20140506 Enabled: true ID: abhere2@moztw.org Name: Add to Search Bar Version: 2.5 Enabled: true ID: add-to-searchbox@maltekraus.de Name: Add-ons Manager Context Menu Version: 0.4.2 Enabled: true ID: amcontextmenu@loucypher Name: Addons Manager Hilite Version: 2.1.1 Enabled: true ID: addonsmgrhilte@cfl Name: Ageless Version: 0.7 Enabled: true ID: 2341n4m3@gmail.com Name: BetterPrivacy Version: 1.68 Enabled: true ID: {d40f5e7b-d2cf-4856-b441-cc613eeffbe3} Name: Browser View Version: 34 Enabled: true ID: {82129504-17c6-4fec-b132-9c17e61879ca} Name: Change Bookmark Recent Folder List Length Version: 1.1.0 Enabled: true ID: change-bookmark-folder-list-length@makyen.foo Name: Classic Theme Restorer Version: 1.2.4 Enabled: true ID: ClassicThemeRestorer@ArisT2Noia4dev Name: Click to Play per-element Version: 0.0.7 Enabled: true ID: ClickToPlayPerElement@uaSad.addons.mozilla.org Name: Click-to-Play Manager Version: 1.3.1 Enabled: true ID: click-to-play-manager@xulforge.com Name: Configuration Mania Version: 1.19.2014091501 Enabled: true ID: {c4d362ec-1cff-4ca0-9031-99a8fad7995a} Name: Context Search X Version: 0.4.6.16 Enabled: true ID: contextsearch2@lwz.addons.mozilla.org Name: Cookie Controller Version: 3.7 Enabled: true ID: {ac2cfa60-bc96-11e0-962b-0800200c9a66} Name: Distill Web Monitor - AlertBox Version: 1.1.1 Enabled: true ID: alertbox@ajitk.com Name: DownloadHelper Version: 4.9.24 Enabled: true ID: {b9db16a4-6edc-47ec-a1f4-b86292ed211d} Name: Duplicate in Tab Context Menu Version: 1.0.9 Enabled: true ID: DuplicateInTabContext@schuzak.jp Name: GPU Accelerated Flash Player Version: 1.32 Enabled: true ID: gpuacceleratedflashplayer@stas Name: Greasemonkey Version: 2014.10.10.nightly Enabled: true ID: {e4a8a97b-f2ed-450b-b12d-ee082ba24781} Name: HttpFox Version: 0.8.14 Enabled: true ID: {4093c4de-454a-4329-8aff-c6b0b123c386} Name: Link Properties Plus Version: 1.5.3pre6 Enabled: true ID: linkPropertiesPlus@infocatcher Name: Open in Browser Version: 1.15 Enabled: true ID: openinbrowser@www.spasche.net Name: OPIE Version: 5.0.1 Enabled: true ID: OPIE@guid.customsoftwareconsult.com Name: Precise Clear History Version: 1.3 Enabled: true ID: preciseclearhistory@vano Name: Print Edit Version: 12.10 Enabled: true ID: printedit@DW-dev Name: Private Tab Version: 0.1.7.3 Enabled: true ID: privateTab@infocatcher Name: ProfileSwitcher Version: 1.6.2 Enabled: true ID: {fa8476cf-a98c-4e08-99b4-65a69cb4b7d4} Name: QuickJava Version: 2.0.4 Enabled: true ID: {E6C1199F-E687-42da-8C24-E7770CC3AE66} Name: Redirect Cleaner Version: 2.4.0 Enabled: true ID: redirectcleaner@example.net Name: Redirector Version: 2.8.3 Enabled: true ID: redirector@einaregilsson.com Name: RefControl Version: 0.8.16 Enabled: true ID: {455D905A-D37C-4643-A9E2-F6FEFAA0424A} Name: RequestPolicy Version: 0.5.28 Enabled: true ID: requestpolicy@requestpolicy.com Name: Restartless Restart Version: 9 Enabled: true ID: restartless.restart@erikvold.com Name: ScrapBook Version: 1.5.11 Enabled: true ID: {53A03D43-5363-4669-8190-99061B2DEBA5} Name: Session Exporter Version: 0.9 Enabled: true ID: {943b5589-7808-4a70-acdc-7b6ee21e7cce} Name: Session Manager Version: 0.8.1.6pre20140929b Enabled: true ID: {1280606b-2510-4fe0-97ef-9b5a22eafe30} Name: SettingSanity Version: 0.8.2 Enabled: true ID: {12A60D0F-0077-4F41-81B2-1286DDD278BB} Name: Sidebars List Version: 0.2.1 Enabled: true ID: sidebarsList@infocatcher Name: Silent Block Version: 1.2.2 Enabled: true ID: SilentBlock@schuzak.jp Name: Star-Button In Urlbar Version: 1.1 Enabled: true ID: Starbuttoninurlbar@ArisT2Noia4dev Name: Tab in Textarea Version: 0.10.2 Enabled: true ID: {df9815d2-ca91-4e8f-aaa3-066d1de2c7a2} Name: Table2Clipboard Version: 1.5.3.1 Enabled: true ID: {9ab67d74-ec41-4cb2-b417-df5d93ba1beb} Name: Textarea Cache Version: 0.9.3.2 Enabled: true ID: {578e7caa-210f-4967-a0d3-88fe5b59a39f} Name: Tile Tabs Version: 11.12 Enabled: true ID: tiletabs@DW-dev Name: UAControl Version: 0.1.3.1 Enabled: true ID: uacontrol@qz.tsugumi.org Name: URL Alias Version: 2.3.2 Enabled: true ID: urlalias@zibada.xgm.ru Name: Video Blocker Version: 1.0.0 Enabled: true ID: jid1-3OQ5HY7YsLBV7Q@jetpack Name: What about:.. Version: 4.1.1 Enabled: true ID: jid0-IPSuVKD0J7yL1cIBwQAdoHTCWmY@jetpack Name: YesScript Version: 2.0 Enabled: true ID: yesscript@userstyles.org Name: YouTube Grid Search and Preview Player Version: 3.7.0 Enabled: true ID: gridtube@gridtube.txt Name: YouTube High Definition Version: 32.1 Enabled: true ID: {7b1bf0b6-a1b9-42b0-b75d-252036438bdc} Name: 1-Click YouTube Video Downloader Version: 2.3.6 Enabled: false ID: YoutubeDownloader@PeterOlayev.com Name: Adblock Edge Version: 2.1.5 Enabled: false ID: {fe272bd1-5f76-4ea4-8501-a05d35d823fc} Name: Adblock Plus Version: 2.6.4.3867 Enabled: false ID: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} Name: Auto Shutdown NG Version: 0.9.16 Enabled: false ID: jid0-HZ5UvAEiWWAxT9TKLuhEgUCARqo@jetpack Name: BehindTheOverlay Version: 0.1.1 Enabled: false ID: jid1-Y3WfE7td45aWDw@jetpack Name: Bluhell Firewall Version: 2.4.0 Enabled: false ID: {6BB5760D-F97E-421B-AF5B-8457A90C3CED} Name: ChatZilla Version: 0.9.91 Enabled: false ID: {59c81df5-4b7a-477b-912d-4e0fdf64e5f2} Name: Click to Play per-element CE Version: 0.0.5 Enabled: false ID: ClickToPlayPerElementCE@rpzrpz123.addons.mozilla.org Name: Download Plan Version: 1.3 Enabled: false ID: downloadplan@firefoxmania.uci.cu Name: Element Hiding Helper for Adblock Plus Version: 1.3 Enabled: false ID: elemhidehelper@adblockplus.org Name: FEBE Version: 8.0.5 Enabled: false ID: {4BBDD651-70CF-4821-84F8-2B918CF89CA3} Name: FireSSH Version: 0.94.5 Enabled: false ID: firessh@nightlight.ws Name: FireX Proxy Version: 3.3 Enabled: false ID: divanproger@gmail.com Name: Flash and Video Download Version: 1.62 Enabled: false ID: {bee6eb20-01e0-ebd1-da83-080329fb9a3a} Name: Flash Video Downloader - YouTube Full HD Download Version: 6.1.2 Enabled: false ID: artur.dubovoy@gmail.com Name: Flashblock Version: 1.5.17 Enabled: false ID: {3d7eb24f-2740-49df-8937-200b1cc08f8a} Name: FlashGot Version: 1.5.6.7 Enabled: false ID: {19503e42-ca3c-4c27-b1e2-9cdb2170ee34} Name: FlashStopper Version: 1.2.3 Enabled: false ID: flashstopper@byo.co.il Name: Form History Control Version: 1.3.3.0 Enabled: false ID: formhistory@yahoo.com Name: FoxReplace Version: 0.16.1 Enabled: false ID: fox@replace.fx Name: Free Download Manager plugin Version: 1.6.0.6 Enabled: false ID: fdm_ffext@freedownloadmanager.org Name: Globefish Version: 1.4.1 Enabled: false ID: globefish@projects.6831.courses.csail.mit.edu Name: Google Reverse Image Search Version: 32.0 Enabled: false ID: {95322c08-05ff-4f3c-85fd-8ceb821988dd} Name: Google search link fix Version: 1.4.9 Enabled: false ID: jid0-XWJxt5VvCXkKzQK99PhZqAn7Xbg@jetpack Name: Hide Plugin Notifications Version: 0.2 Enabled: false ID: suppress-plugin-infobar@benjamin.smedbergs.us Name: Hola Better Internet Version: 1.4.995 Enabled: false ID: jid1-4P0kohSJxU1qGg@jetpack Name: HTTP UserAgent cleaner Version: 0.6.0.6 Enabled: false ID: HTTPUserAgentcleaner@addons.8vs.ru Name: Integrated Authentication for Firefox Version: 3.0.1 Enabled: false ID: extension@firefox-ntlmauth.googlecode.com Name: JavaScript Deobfuscator Version: 1.6.4.196 Enabled: false ID: jsdeobfuscator@adblockplus.org Name: Kill Infinite Scroll Version: 0.4 Enabled: false ID: killInfiniteScroll@jetpack Name: KillSpinners Version: 1.2.0 Enabled: false ID: killspinners@byo.co.il Name: LeechBlock Version: 0.6.6 Enabled: false ID: {a95d8332-e4b4-6e7f-98ac-20b733364387} Name: OneTab Version: 1.9 Enabled: false ID: extension@one-tab.com Name: Open Link in Silent Tab Version: 0.0.6 Enabled: false ID: {d4c46ca0-999d-11da-a72b-0800200c9a66} Name: Referrer Control Version: 0.4.5 Enabled: false ID: referrercontrol@qixinglu.com Name: ReloadEvery Version: 28.0.2 Enabled: false ID: {888d99e7-e8b5-46a3-851e-1ec45da1e644} Name: Screengrab (fix version) Version: 0.98.02c Enabled: false ID: {02450914-cdd9-410f-b1da-db004e18c671} Name: Search term highlighter Version: 7.1 Enabled: false ID: {458482f0-90fb-4257-855f-0ba2790584f9} Name: SpeedView Version: 1.0 Enabled: false ID: jid1-MmDjnsjlez2Sdw@jetpack Name: Sticky Notes Version: 0.7.2 Enabled: false ID: sticky@filenamezero.dip.jp Name: Stylish Version: 1.4.3 Enabled: false ID: {46551EC9-40F0-4e47-8E18-8E5CF550CFB8} Name: Tab Grenade Version: 0.8.5 Enabled: false ID: jid1-gzlHTgBCb5hzkA@jetpack Name: Tab Utilities Version: 1.5.28.1 Enabled: false ID: tabutils@ithinc.cn Name: TheSage one-click lookup Version: 5.0.4406 Enabled: false ID: wcapturex@deskperience.com Name: UnMHT Version: 7.2.0.2 Enabled: false ID: {f759ca51-3a91-4dd1-ae78-9db5eee9ebf0} Name: YouTube Center Version: 2.1.1 Enabled: false ID: jid1-cwbvBTE216jjpg@jetpack Graphics -------- Adapter Description: ATI Radeon HD 4290 (Microsoft Corporation - WDDM v1.1) Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64 Adapter RAM: 368 Device ID: 0x9714 Direct2D Enabled: true DirectWrite Enabled: true (6.3.9600.17111) Driver Date: 6-20-2012 Driver Version: 8.97.10.6 GPU #2 Active: false GPU Accelerated Windows: 4/4 Direct3D 11 (OMTC) Subsys ID: 00000000 Vendor ID: 0x1002 WebGL Renderer: Google Inc. -- ANGLE (ATI Radeon HD 4290 (Microsoft Corporation - WDDM v1.1) Direct3D9Ex vs_3_0 ps_3_0) windowLayerManagerRemote: true AzureCanvasBackend: direct2d AzureContentBackend: direct2d AzureFallbackCanvasBackend: cairo AzureSkiaAccelerated: 0 Important Modified Preferences ------------------------------ accessibility.typeaheadfind.flashBar: 0 browser.cache.disk.capacity: 358400 browser.cache.disk.smart_size_cached_value: 358400 browser.cache.disk.smart_size.first_run: false browser.cache.disk.smart_size.use_old_max: false browser.cache.frecency_experiment: 3 browser.newtab.url: about:blank browser.places.smartBookmarksVersion: 7 browser.sessionstore.max_tabs_undo: 30 browser.sessionstore.max_windows_undo: 10 browser.sessionstore.upgradeBackup.latestBuildID: 20141011030203 browser.startup.homepage_override.buildID: 20141011030203 browser.startup.homepage_override.mstone: 35.0a1 browser.tabs.defaultDrawInTitlebar: false browser.tabs.drawInTitlebar: false browser.urlbar.autoFill: false browser.urlbar.unifiedcomplete: false dom.mozApps.used: true extensions.lastAppVersion: 35.0a1 gfx.canvas.azure.backends: direct2d,skia,cairo gfx.content.azure.backends: direct2d,cairo gfx.direct2d.use1_1: false gfx.direct3d.last_used_feature_level_idx: 0 image.animation_mode: none media.autoplay.enabled: false media.gmp-gmpopenh264.lastUpdate: 1412184805 media.gmp-gmpopenh264.path: D:\Downloads\debug\tmp\gmp-gmpopenh264 media.gmp-gmpopenh264.version: 1.1 media.gmp-manager.lastCheck: 1413295659 network.cookie.lifetimePolicy: 2 network.cookie.prefsMigrated: true network.dns.disablePrefetch: true network.prefetch-next: false places.database.lastMaintenance: 1413338394 places.history.expiration.transient_current_max_pages: 104858 plugin.disable_full_page_plugin_for_types: application/pdf,application/vnd.adobe.xfdf,application/vnd.fdf,application/vnd.adobe.xdp+xml plugin.importedState: true plugin.persistentPermissionAlways.intervalInDays: 0 plugin.sessionPermissionNow.intervalInMinutes: 0 plugin.state.flash: 1 plugin.state.java: 0 privacy.cpd.cookies: false privacy.cpd.extensions-betterprivacy: true privacy.cpd.extensions-sessionmanager: false privacy.cpd.historysession: false privacy.cpd.sessions: false privacy.sanitize.migrateFx3Prefs: true storage.vacuum.last.index: 1 storage.vacuum.last.places.sqlite: 1412626038 Important Locked Preferences ---------------------------- JavaScript ---------- Incremental GC: true Accessibility ------------- Activated: false Prevent Accessibility: 0 Library Versions ---------------- NSPR Expected minimum version: 4.10.7 Version in use: 4.10.7 NSS Expected minimum version: 3.17.2 Basic ECC Version in use: 3.17.2 Basic ECC NSSSMIME Expected minimum version: 3.17.2 Basic ECC Version in use: 3.17.2 Basic ECC NSSSSL Expected minimum version: 3.17.2 Basic ECC Version in use: 3.17.2 Basic ECC NSSUTIL Expected minimum version: 3.17.2 Version in use: 3.17.2 Experimental Features --------------------- Name: Invisible test of the experiment branching system. ID: experiment-branch-test-nightly@experiments.mozilla.org Description: An experiment using branches just to test whether branches get saved correctly. Active: false End Date: 1409591246458 Homepage:

The console spew is related to bug 1082850. If that's the problem, the patch we get there should solve it. We should have a build we can test of that tomorrow.

Backed out of Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/d56d0157891e

TM tracks trunk landing, so leave that as-is unless you backout from m-c :)

Could I get you to try a build? Debug: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jgilbert@mozilla.com-471be9c96fae/try-win32-debug/firefox-36.0a1.en-US.win32.zip Opt: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jgilbert@mozilla.com-471be9c96fae/try-win32/firefox-36.0a1.en-US.win32.zip

I'll look at it tomorrow (might not been till the evening), as I'm going to bed now. Meanwhile, if I won't be able to run the debug or opt builds without special configurations, is there a regular build available? In the past I have never succeeded in getting a debug build to start (it always required something I couldn't figure out -- even after reading the confusing documentation), so I stay away from them. Supposedly their easier to use now, but I haven't touched one for probably three years now. Thanks

(In reply to IU from comment #59) > I'll look at it tomorrow (might not been till the evening), as I'm going to > bed now. > > Meanwhile, if I won't be able to run the debug or opt builds without special > configurations, is there a regular build available? In the past I have > never succeeded in getting a debug build to start (it always required > something I couldn't figure out -- even after reading the confusing > documentation), so I stay away from them. > > Supposedly their easier to use now, but I haven't touched one for probably > three years now. > > Thanks I run them similar to: > mkdir foo 2>/dev/null > > export JS_DISABLE_SLOW_SCRIPT_SIGNALS=1 > export MOZ_QUIET=1 > ./dist/bin/firefox -console -profile foo -no-remote But even just running `firefox` should work, if you don't have another browser window open.

Before leaving for work, I managed to test the debug build with the same video I mentioned earlier. There is no more crashing or spewing of errors. The fix seems to work, as my GPU did not become unstable afterwards (as was the case with the buggy builds). One thing though, I used my default profile to run the debug build and now debugging in permanently turned on and I get crashing when subsequently launching Firefox. How do I completely disable debugging from my profile?

Forget the last part, something strange was happening -- even after reverting to a non-debug build. So I completely deleted and extracted a new build and now my profile is fine.