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

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

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?

Comment 2

[Nicolas Silva [:nical]]

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.

Comment 3

[Chris Peterson [:cpeterson]]

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?

Comment 4

[Nicolas Silva [:nical]]

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.

Comment 5

[Chris Peterson [:cpeterson]]

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". <:)

Comment 6

[Nicolas Silva [:nical]]

Attachment: Fixes fennec blank video.

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

Comment 7

[Chris Peterson [: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").

Comment 8

[Nicolas Silva [:nical]]

(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.

Comment 9

[Nicolas Silva [:nical]]

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

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b349285eff68

Comment 11

[Chris Peterson [:cpeterson]]

VERIFIED FIXED on Nightly 18.0a1 2012-10-01