863498 - Embedded YouTube Flash video is upside down, though the player controls are oriented correctly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Plugins, defect)

Description

[Chris Peterson [:cpeterson]]

When playing the following embedded YouTube Flash video on my Nexus 4, the video is upside down, though the YouTube player controls are oriented correctly.

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

This is a regression! The video orientation is correct in Aurora 22 (2013-04-18), but not in Nightly 23 (2013-04-18).

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Screenshot_before_video_playback.jpg

Here is a screenshot of the upside-down video and player controls.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Screenshot_during_video_playback.jpg

Here is a screenshot of the upside-down video with correctly-oriented player controls after the video starts playing.

Comment 3

[Chris Peterson [:cpeterson]]

btw, snorp's suggestion from bug 827407 comment 20 did not fix the problem.

> > 
> > 2. If I ignore the SurfaceCaps assertions, the video starts playing but the
> > image is upside down! Panning the page fixes the video orientation.
> > 
> 
> Hmm. It sounds like we're missing a layer invalidation. Maybe try 
> nsNPAPIPluginInstance::RedrawPlugin() after calling SetInverted in 
> anp_native_window_invertPluginContent() (ANPNativeWindow.cpp)

Comment 4

[Chris Peterson [:cpeterson]]

This is a regression between Nightly 23 build 2013-04-09 and 2013-04-16. Unfortunately, the regression range spans 7 days because every nightly build from 04-10 through 04-15 crashes when trying to play this video. Build 04-16 doesn't crash, but the video is upside down. The upside-down video bug is likely related to the video crash or the crash's fix.

Here is the pushlog from build 04-09 to 04-10 that introduced the video crash:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=ee5ca214e87c

Here is the pushlog from build 04-15 to 04-16 which fixed the video crash:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=261d6997d1d1&tochange=1d9c510b3742

Here is the entire pushlog from build 04-09 to 04-16:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=1d9c510b3742

Comment 5

[Scoobidiver (away)]

Crashes are bug 860466 (regression from bug 825928) then bug 861796 (regression from bug 802240). Back out bug 825928 and bug 802240 to narrow down the regression range. Bug 825928 might be the culprit.

Comment 6

[Chris Peterson [:cpeterson]]

Thanks, Scoobidiver! I have confirmed that this upside-down video bug is a regression from either bug 825928 or related crash fix bug 860466.

Comment 7

[Chris Peterson [:cpeterson]]

If I play both videos on this page [1] simultaneously, the first video is upside-down, but the second video is right-side up.


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

Comment 8

[Chris Peterson [:cpeterson]]

snorp or jgilbert:

Do you have any suggestions for this bug? Does the comment about TEXTURE_EXTERNAL from bug 860466 comment 6 apply here?

Comment 9

[u421692]

We could not find an acceptable regression range because the app crashes between Nightly 23.0a1 (2013-04-10) and Nightly 23.0a1 (2013-04-15.). 
Good build: Nightly 23.0a1 (2013-04-09)
Bad build: Nightly 23.0a1 (2013-04-16)
The pushlog is: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=1d9c510b3742

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> snorp or jgilbert:
> 
> Do you have any suggestions for this bug? Does the comment about
> TEXTURE_EXTERNAL from bug 860466 comment 6 apply here?

I don't think TEXTURE_EXTERNAL has anything to do with it. If that part wasn't working, you wouldn't get anything at all on the screen.

I think it's likely the value of nsNPAPIPluginInstance::Inverted() is getting dropped at some point up the rendering chain.

Comment 11

[u421692]

Attachment: Samsung Galaxy Tab 2(GT-P3113)

Beside that the image is upside down on a Embedded YouTube Flash video, on Samsung Galaxy Tab 2(GT-P3113) there is noise around the video.
Nightly 23.0a1 (2013-04-28)

Comment 12

[Chris Peterson [:cpeterson]]

I can reproduce this video bug on ICS and JB, but not GB.

Comment 13

[Chris Peterson [:cpeterson]]

Bug 786248 describes a similar video issue that was fixed on Honeycomb.

Comment 14

[Chris Peterson [:cpeterson]]

kbrosnan says he could not able to reproduce the problem on HC.

Comment 15

[Chris Peterson [:cpeterson]]

Track for 23? We can't ship upside-down video. :)

Comment 16

[Chris Peterson [:cpeterson]]

jrmuizel, this Android rendering bug is a regression from the big layers refactoring (bug 825928). blassey suggested I forward this bug to you.

* Some background on the bug:

The basic problem is Flash video is upside-down on Android until the user resizes the page (causing a repaint). 

Curiously, if I watch an upside-down Flash video without resizing the page, load a new page with a (non-video) Flash animation, then the new Flash animation is also upside-down. I can refresh the page multiple times, but the Flash animation is still upside-down until I resize the page (causing a repaint). There seems to be some layer state cached or remembered across pages, even for different Flash plugin instances on different pages. 

I wonder if this is a race condition because even non-video Flash animations will sometimes be rendered upside-down if I linger at a breakpoint in nsPluginInstanceOwner::GetImageContainer(). GetImageContainer() seems to get called once before the Flash plugin calls our anp_native_window_invertPluginContent(true) NPAPI, so GetImageContainer() sees SharedTextureImage::Data with mInverted = false. Later calls to GetImageContainer() see mInverted = true. Android is using MOZ_ANDROID_OMTC. 


* mwoodrow suggested:

A race condition seems likely, but I can't see where it would be happening. When the plugin updates, we should hit this code (on the main thread) which will send the value of mInverted to the compositor. I'd expect this to maybe happen every frame, and at least when the value of mInverted changes.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ImageClient.cpp#145

On the compositor end we should hit this code:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#251

Which looks like it updates the flipped state if it changes, though it looks like it would fail to remove it (which shouldn't be relevant to your bug).

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I took a look at this and it's not clear to me why things are being inverted the way they are. To the compositor it seems like once we get the call to anp_native_window_invertPluginContent() both ImageContainers send there updates with inverted set to true. This makes it through to the compositor.

What I don't understand is why when we do a repaint, things are fixed. From the compositor's point of view the inverted flag is still set on both Images. It seems as though flash decides to flip the drawing. I also didn't see any additional calls to inverPluginContent() to match this change.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

So it looks like this is caused by inconsistency of the surface texture transform. In fact it looks like we're not using that transform at all.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: Ignoring Inverted and TextureSurface transform produces better results

Comment 20

[Jeff Muizelaar [:jrmuizel]]

So the bug here is that we're not getting the TextureTransform every time we draw.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Attachment: A better patch

This makes sure we read the TextureSurface transform everytime we draw it.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 747126 [details] [diff] [review]
A better patch

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

Yeah this looks much better

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Attachment: Get the transform everytime that we draw

ShadowImageLayerOGL::RenderLayer used to call gl()->GetSharedHandleDetails() directly and uses that result.

This makes us do something similar. As a concequence we also drop the mTextureTransform member because we are not caching it anymore.

I do wonder if it would make more sense for the content thread to send the transform along with the update to make sure that it always matches. This would also make it so that we have no chance of causing a GC in the compositor to get the transform by calling through java.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

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

Comment 25

[Ed Morley [:emorley]]

Backed out for compilation failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a23a23055e13

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59fe6fbf1adf

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Try again with support for compilers supporting 'override'

https://hg.mozilla.org/integration/mozilla-inbound/rev/65129bb74325

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/65129bb74325

Comment 28

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0b8(2013-07-23)
Device: Samsung Galaxy Tab2
Android: 4.1.1