On my Retina MBP, embedded YouTube videos only show the top-left quadrant. If I disable HiDPI support by setting the gfx.hidpi.enabled pref to -1, then the embedded videos are displayed correctly. Example video: http://www.theguardian.com/commentisfree/2013/aug/21/sending-message-miranda-gchq-nsa

Matt, does this bug look like a regression from OSX texture bug 900133? This is a regression in Nightly build 2013-08-21. I bisected the TBPL builds from mozilla-inbound from the prior day and the regression range is includes bug 900133: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0f68b32be429&tochange=252dbb41d609

Looks like a bug 896250, which was caused by non-active plugin layers incorrectly applying the content scale (captured in bug 900640.) And it looks like one of the changes in the above range caused that to happen?

Workaround - set layers.use-deprecated-textures to true.

This lets the texture client decide how it needs to be released, rather than the compositable. This makes more sense when we add the following patch. It also fixes a bug in ShouldDeallocateInDestructor.

This is what we were using with old textures. I don't have a retina MBP to test, but I think this will fix the size issue too.

This isn't the first time this sort of regression has happened. Is there any way we can get this in the smoketest or write a automated test case for it?

Comment on attachment 794447 [details] [diff] [review] Use SharedTextureImage again Review of attachment 794447 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ImageClient.cpp @@ +178,5 @@ > + RemoveTextureClient(mFrontBuffer); > + mFrontBuffer = nullptr; > + } > + > + mFrontBuffer = new SurfaceDescriptorTextureClient(SurfaceDescriptor(texture), Why not use SharedTextureClientOGL? ::: gfx/layers/client/TextureClient.h @@ +281,5 @@ > uint8_t* mBuffer; > size_t mBufSize; > }; > > +class SurfaceDescriptorTextureClient : public TextureClient I really really want to avoid this. Exposing SurfaceDescriptor outside of the IPC glue is a big source of problems and I am putting a lot of efforts into removing the uses of SurfaceDescriptors all over the place. If you want a new texture client that accepts a memory type that we don't support already, please add a specific TextureClient class that offers proper memory management.

(In reply to Doug Turner (:dougt) from comment #6) > This isn't the first time this sort of regression has happened. Is there > any way we can get this in the smoketest or write a automated test case for > it? I agree, these breakages are really annoying. It would be really good to have test coverage for this. Comment 1 suggests that gfx.hidpi.enabled can prevent the bug, so I'd hope it's possible to force hidpi on a regular system to reproduce it.

It's definitely possible to fake HiDPI mode on system that don't have a retina display, see http://stackoverflow.com/questions/12124576/how-to-simulate-a-retina-display-hidpi-mode-in-mac-os-x-10-8-mountain-lion-on. It'd be great to get test machines set up that way, and start running tests on them. I'll file a bug!

Bug 859742 is for solving any current test issues running in HiDPI mode, btw.

I filed bug 909202.

I didn't even notice that you'd already written this class, and just hacked up a generic one instead. You're right, this is much better in all ways :) Seems to work with 'fake retina' mode on my non-retina MBP.

Comment on attachment 795735 [details] [diff] [review] Use SharedTextureImage again v2 Review of attachment 795735 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ImageClient.cpp @@ +179,5 @@ > + buffer->InitWith(data->mHandle, size, data->mShareType, data->mInverted); > + mFrontBuffer = buffer; > + > + AddTextureClient(mFrontBuffer); > + GetForwarder()->UseTexture(this, mFrontBuffer); You may be willing to fix this regression asap so I'll accept this patch but could you do a followup that removes this chunk and implements AsSharedImage() and GetTextureClient() for SharedTextureImage instead? That's the right way to do it and it avoids accumulating big chunks of if-else code in UpdateImage.

(In reply to Nicolas Silva [:nical] from comment #17) > https://hg.mozilla.org/integration/mozilla-inbound/rev/a255d2d2b67f Backed out for OSX asserts. Can we please make sure that these have run through Try before pushing? https://hg.mozilla.org/integration/mozilla-inbound/rev/436d3fb82455 https://tbpl.mozilla.org/php/getParsedLog.php?id=27069051&tree=Mozilla-Inbound

Comment on attachment 796090 [details] [diff] [review] Implement GetTextureClient in SharedTextureImage Review of attachment 796090 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/SharedTextureImage.h @@ +51,5 @@ > + { > + if (!mTextureClient) { > + // do not use the flags TEXTURE_DEALLOCATE_CLIENT nor TEXTURE_DEALLOCATE_HOST > + // because the shared data is externally owned. > + mTextureClient = new SharedTextureClientOGL(TEXTURE_FRONT); Interesting. I feel that flags like TEXTURE_FRONT probably should be a decision made by the compositable, yet flags like TEXTURE_DEALLOCATE_HOST should be decided by the texture. Maybe we need two sets of flags sometime in the future to differentiate between these two use cases.

https://tbpl.mozilla.org/?tree=Try&rev=4e6099b835e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d0dd73fc25 https://hg.mozilla.org/integration/mozilla-inbound/rev/53b6481108a6 (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19) > (In reply to Nicolas Silva [:nical] from comment #17) > > https://hg.mozilla.org/integration/mozilla-inbound/rev/a255d2d2b67f > > Backed out for OSX asserts. Can we please make sure that these have run > through Try before pushing? > https://hg.mozilla.org/integration/mozilla-inbound/rev/436d3fb82455 > > https://tbpl.mozilla.org/php/getParsedLog.php?id=27069051&tree=Mozilla- > Inbound Sorry about this. We did have a try run, but I didn't make it clear that we needed both patches landed for it to pass.

(In reply to Matt Woodrow (:mattwoodrow) from comment #20) > Comment on attachment 796090 [details] [diff] [review] > Implement GetTextureClient in SharedTextureImage > > Review of attachment 796090 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/SharedTextureImage.h > @@ +51,5 @@ > > + { > > + if (!mTextureClient) { > > + // do not use the flags TEXTURE_DEALLOCATE_CLIENT nor TEXTURE_DEALLOCATE_HOST > > + // because the shared data is externally owned. > > + mTextureClient = new SharedTextureClientOGL(TEXTURE_FRONT); > > Interesting. I feel that flags like TEXTURE_FRONT probably should be a > decision made by the compositable, yet flags like TEXTURE_DEALLOCATE_HOST > should be decided by the texture. > > Maybe we need two sets of flags sometime in the future to differentiate > between these two use cases. Yes, makes sense. Btw let's not (as I did ealier) rush to land this patch, i just built it but I didn't test it or pushed to try yet.

https://hg.mozilla.org/mozilla-central/rev/a7d0dd73fc25 https://hg.mozilla.org/mozilla-central/rev/53b6481108a6

(In reply to Cameron McCormack (:heycam) from comment #11) > Bug 859742 is for solving any current test issues running in HiDPI mode, btw. That's an excellent end-goal, but I think we'd also get a lot of value out of even just a quickie/one-off plugin-specific test (since that seems to be the thing that keeps regressing)... Flip the pref to force hidpi, load a page with the test plugin, and see if it's ok (either as a reftest, or manually drawWindow it to a canvas and check a few pixel values). The alternative is to be more aggressive about backing out breakage, but that still means someone has to hunt down the regressor and verify the backout.

I just want to report that the problem is fixed in (or before) "26.0a1 (2013-08-31)".

I am still getting this behaviour on today's Nightly on a MBP Retina running OS X 10.9 when playing vidoes on http://video.pbs.org.

I get it when I switch to full screen: http://www.heavy.com/entertainment/2013/11/ylvis-dwts-finale-performance-dancing-with-the-stars/

I'm still seeing this on the latest Beta...?

Jonathan: You're probably not seeing *this* bug, which was fixed long ago. Please open another bug with as much detail as possible. Exactly which videos? What version of OS X? What hardware (a Retina MacBook Pro?)? Does it have an external monitor attached? Detailed steps to reproduce.

I'm trying to figure out the repro steps. I'll open a bug once I have enough info.