Closed Bug 794233 Opened 8 years ago Closed 8 years ago

Stagefright video playback is blank in release builds, but not debug builds

Categories

(Core :: Audio/Video, defect)

18 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox17 --- unaffected
firefox18 --- fixed

People

(Reporter: cpeterson, Assigned: nical)

References

Details

(Keywords: regression)

Attachments

(1 file)

I ran into a strange problem when debugging some video bugs on my Galaxy Nexus. 

Using that latest mozilla-central or mozilla-inbound, H264 video is not rendered to the screen in release builds. If I add --enable-debug to my mozconfig, then the video shows up. Have you seen a problem like this before?

I can still hear the audio and, if I look at Firefox's tab list (which includes thumbnails of each tab), I see a snapshot of the expected video in the thumbnail!

This is not an optimized vs unoptimized build problem because I compile my debug builds with --enable-optimize. So I'm guessing there is some video or gfx code that is #ifdef'd DEBUG that should not be.

Nightly 2012-09-19 seems to be the regression build. Here is the pushlog from build 09-18 to 09-19:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0d3b17a88d5f&tochange=80499f04e875
I have identified cset cc9e531de8f2 (bug 790716) as the regression.

Nicolas, do you know why your ShmemYCbCrImage change might cause Android video to render correctly in debug (but still optimized) builds of Firefox, but not in release builds?
Blocks: 790716
Hm strange, try push was all green and I don't recall adding debug specific code. Is this only with Stagefright videos? I usually test my code on a webm video.
I'm looking at it now.
Nicolas, I'm testing Stagefright's hardware and software decoders for H264, but I can repro the problem with both webm and H264 videos.

What is the difference between SharedImage::TYCbCrImage and SharedImage::TYUVImage?

Fennec is still asking ImageContainerChild for TYUVImage. If I resurrect the old TYUVImage copy code and add something like `switch (dest->type()) { case TYCbCrImage: ... case TYUVImage: ... }`, then video plays again.

Do you expect that TYCbCrImage should replace all TYUVImage code?
Interesting! Thanks for finding this. So basically YCbCrImage is meant to replace YUVImage entirely. The thing is that YUVImage is still used in the OMTC code paths that don't use async-video, and replacing YUVImage with YCbCrImage there means changing a lot of code in all the ImageLayer classes, which would completely bitrot the layers refactoring that is going on (and should land soon hopefully). So I chose to not replace YUVImage outside the async-video codepath for now.

Normally these code paths should not be used when we are using async-video, so we should not use any YUVImage on fennec.

The difference between the two is that YUVImage uses 3 shmems per image, (this is making us run into a limit of open file descriptors when we play many videos at the same time) while YCbCrImage uses one shmem per frame and could also use one shmem for several frames if need be.

It is weird that the behaviour is different between debug and release builds, but i can reproduce the bug so i'll find out soon and fix it.
Thanks for your help, Nicolas. This is a blocking bug for Android video testing because we can't play any videos.

I'm not sure why debug and release builds would behave differently. I toggled debug/release three times to be sure, but perhaps my test results were just "user error". <:)
Assignee: cpeterson → nsilva
Severity: normal → major
Priority: -- → P1
I think I found it \o/

I thought NS_ABORT_IF_FALSE was kept in release builds which it is not, so trying to open a shmem as an image was always failing because the Open(shm,offset) call was an argument of NS_ABORT_IF_FALSE.

try push: https://tbpl.mozilla.org/?tree=Try&rev=0f28149b3210
Attachment #665174 - Flags: review?(cpeterson)
Comment on attachment 665174 [details] [diff] [review]
Fixes fennec blank video.

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

LGTM with comments. Thanks for your help, Nicolas!

::: gfx/layers/ipc/ShmemYCbCrImage.h
@@ +35,5 @@
>  
>    ShmemYCbCrImage() : mOffset(0) {}
>  
>    ShmemYCbCrImage(Shmem& shm, size_t offset = 0) {
> +    bool status = Open(shm,offset);

nit: some compilers might warn about the status variable being unused in release builds. You can use mozilla/Util.h's mozilla::DebugOnly<bool> template to avoid the release warning.

@@ +36,5 @@
>    ShmemYCbCrImage() : mOffset(0) {}
>  
>    ShmemYCbCrImage(Shmem& shm, size_t offset = 0) {
> +    bool status = Open(shm,offset);
> +    NS_ASSERTION(status, "Invalid data in the shmem");

If NS_ASSERTION() fails, it will only log a warning. If you want debug builds to abort (if Open() failure is a programmer bug), then you can use MOZ_ASSERT(status, "Invalid data in the shmem").
Attachment #665174 - Flags: review?(cpeterson) → review+
(In reply to Chris Peterson (:cpeterson) from comment #7)
> nit: some compilers might warn about the status variable being unused in
> release builds. You can use mozilla/Util.h's mozilla::DebugOnly<bool>
> template to avoid the release warning.

Yes, you are right. I changed it to use DebugOnly<bool> instead.

> 
> If NS_ASSERTION() fails, it will only log a warning. If you want debug
> builds to abort (if Open() failure is a programmer bug), then you can use
> MOZ_ASSERT(status, "Invalid data in the shmem").

Yes and actually I want to keep this behaviour. Aborting here was a mistake as the users of ShmemYCbCrImage should use IsValid() to check and decide what to do in case of failure.
https://hg.mozilla.org/mozilla-central/rev/b349285eff68
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
VERIFIED FIXED on Nightly 18.0a1 2012-10-01
Severity: major → normal
Status: RESOLVED → VERIFIED
Priority: P1 → --
Version: unspecified → 18 Branch
You need to log in before you can comment on or make changes to this bug.