Closed Bug 907792 Opened 6 years ago Closed 6 years ago

Embedded YouTube videos only display 1/4 of the video on Retina MBP/OSX

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox25 --- unaffected
firefox26 + fixed
firefox27 --- verified

People

(Reporter: cpeterson, Assigned: mattwoodrow)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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
Component: Layout → Graphics: Layers
Flags: needinfo?(matt.woodrow)
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?
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Blocks: 900133
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.
Attachment #794445 - Flags: review?(nical.bugzilla)
Attached patch Use SharedTextureImage again (obsolete) — Splinter Review
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.
Attachment #794447 - Flags: review?(nical.bugzilla)
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.
Attachment #794447 - Flags: review?(nical.bugzilla) → review-
Attachment #794445 - Flags: review?(nical.bugzilla) → review+
Duplicate of this bug: 907746
(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.
Attachment #794447 - Attachment is obsolete: true
Attachment #795735 - Flags: review?(nical.bugzilla)
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.
Attachment #795735 - Flags: review?(nical.bugzilla) → review+
Duplicate of this bug: 909784
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.
Attachment #796090 - Flags: review?(matt.woodrow) → review+
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.
(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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla27
Status: RESOLVED → VERIFIED
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.
You need to log in before you can comment on or make changes to this bug.