video playback seems to occasionally use RGB frame instead of YCbCr on Fennec

RESOLVED FIXED in Firefox 15

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

Trunk
mozilla16
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox15 fixed, firefox16 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

While debugging bug 765150, I noticed something weird -- if you change the YCbCr shader so that it just shows one of the incoming textures (e.g. just show the Y channel as gray), and then play a video in Fennec on Android, you'll see a flash of a normal coloured (static) frame every few seconds.  The video will play in grayscale otherwise.

I showed this to jeff; the theory is that when we take our screenshots^H^H^H^Wsnapshots, that switches layers into a different rendering path (down the RGB path since we converted the frame to RGB for software compositing maybe?).  We should probably not do this.  Also, we may want to slow down how frequently we capture when we have things like video playing or requestAnimationFrame animation going on... having a perfectly up to date capture is less important than not causing whatever's going on to stutter!
I disabled screenshots (by setting sDisableScreenshot to true and adding a check for it in notifyPaintedRect since without that we get a stream of NPEs), and I still see the frame or frames painted in color.  This worries me, kind of a lot -- either I didn't /really/ disable screenshots, or we're actually randomly doing the YUV-RGB conversion in software (or maybe all the time) and there's some kind of race condition for the kind of frame we upload?
Created attachment 639190 [details] [diff] [review]
be smarter about marking video as inactive

I think this inactive code was written long before shadow layers and OMTC existed; at least the check for basic doesn't really make sense any more.  For whatever reason, playback of this video is slow on my Tegra 2 phone (720p WebM video, phone with no NEON, who knows) and so playback was jerky.. we'd get 1s or so of playback followed by a pause, followed by 1s or so.  I'm guessing it was buffering decoding?  The layer was being marked as inactive in the meantime, so we were doing the YUV-RGB conversion occasionally in software, causing even more slowdown.

This causes us to check the shadow manager type as well before marking a video layer as inactive.
Assignee: nobody → vladimir
Attachment #639190 - Flags: review?(roc)
Comment on attachment 639190 [details] [diff] [review]
be smarter about marking video as inactive

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

::: layout/generic/nsVideoFrame.cpp
@@ +352,5 @@
>    {
> +    if ((aManager->GetBackendType() != LayerManager::LAYERS_BASIC) ||
> +        (aManager->AsShadowForwarder() &&
> +         aManager->AsShadowForwarder()->HasShadowManager() &&
> +         aManager->AsShadowForwarder()->GetParentBackendType() != LayerManager::LAYERS_BASIC))

I think we can just call aManager->IsCompositingCheap() here
Created attachment 639327 [details] [diff] [review]
be smarter about marking video as inactive

Yup, good idea; didn't see that method.
Attachment #639190 - Attachment is obsolete: true
Attachment #639190 - Flags: review?(roc)
Attachment #639327 - Flags: review?(roc)
Attachment #639327 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09a1fd4e7bd
Target Milestone: --- → mozilla16
Comment on attachment 639327 [details] [diff] [review]
be smarter about marking video as inactive

[Approval Request Comment]
User impact if declined: potentially slower HTML video playback on mobile, since we'll do software YUV->RGB conversion occasionally
Testing completed (on m-c, etc.): tested locally, landing on m-i/m-c
Risk to taking this patch (and alternatives if risky): almost none; just fixes a check that wasn't correct any more
String or UUID changes made by this patch: none
Attachment #639327 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d09a1fd4e7bd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
Comment on attachment 639327 [details] [diff] [review]
be smarter about marking video as inactive

[Triage Comment]
Low risk fix for performance/power consumption. Let's take this on Aurora 15.
Attachment #639327 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/223e26cffc7e
status-firefox15: --- → fixed
status-firefox16: --- → fixed
Target Milestone: mozilla16 → mozilla15

Comment 10

5 years ago
Target Milestone is for m-c.
Target Milestone: mozilla15 → mozilla16
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.