1064376 - Regression in playing few clips on v2.1

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[bhargavg1]

See a regression when playing few clips on v2.1.

we see a black screen for video playback. While the same clip plays on v2.0

v2.1 commits:
gaia - a8e4d26555e5713ec6c72270cfd0cfabc096a0d3
gecko - 8a9db5df79de88e23c04112b4fe8e161d98923de

this issue is blocking our Sanity tests

Comment 1

[bhargavg1]

[Blocking Requested - why for this release]:

Comment 2

[bhargavg1]

Clip shared at, https://drive.google.com/file/d/0B0zTAnPOpx-xTXB6UmtPdkpOUms/edit?usp=sharing

Comment 3

[Jason Smith [:jsmith]]

Can we reproduce this on the Moz side on Flame JB or KK?

Comment 4

[Chris Kreinbring [:CKreinbring]]

Able to repro on KK Flame 2.2 and Flame 2.1
Actual result: Some video clips show a blank black screen when played.

Flame 2.2
BuildID: 20140908062801
Gaia: c71fd5d8c9c7cb021c97e5e9fbb29f92b50a084d
Gecko: f7a27a866c47
Platform Version: 35.0a1
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1
BuildID: 20140908082301
Gaia: 0c808a939741c515a70ab2d649b1e98a14d73f5e
Gecko: 4546e467fcd9
Platform Version: 34.0a2
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

--------------------------------------------------------------------------------------------------------

The bug does not repro on JB Flame 2.1 or KK Flame 2.0
Actual result: All video clips are displayed correctly when playing.

Flame 2.1 (JB)
BuildID: 20140908022757
Gaia: e7ac3a51932f7f7a5b5a6935dcaad1343b7c5fa5
Gecko: d1b97cc46b5a
Platform Version: 34.0a2
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Flame 2.0
BuildID: 20140908082301
Gaia: 35533237253ff5a27e9301a9031c4cc4b1d95eb5
Gecko: 1919c98d39a5
Platform Version: 32.0
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 5

[Hema Koka [:hema]]

Russ will take an initial look. 

Blocking for regression.

Comment 6

[Chris Kreinbring [:CKreinbring]]

The bug repros on the earliest KK build we have available.

BuildID: 20140904171737
Gaia: de59e0c3614dd0061881fe284e9f2d74fa0d1d5d
Gecko: 8703c1895505
Platform Version: 35.0a1
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 7

[Joshua Mitchell (Inactive)]

Comment 6 was meant to indicate that a regression-window is unavailable - we have only recently been storing KK builds and can not get back to a time where there was a "working" build.

Comment 8

[Russ Nicoletti [:russn]]

bhargavg1, please give me access to the video clip on your google drive. Thanks.

Comment 9

[bhargavg1]

(In reply to Russ Nicoletti [:russn] from comment #8)
> bhargavg1, please give me access to the video clip on your google drive.
> Thanks.

Done !

Comment 10

[Jason Smith [:jsmith]]

If this is reproducing on KK but not JB, then my guess is this likely a gecko bug rather than a Gaia bug, as I'd expect a Gaia bug with the video app to be present across JB & KK. Russ - What do you think?

Comment 11

[Russ Nicoletti [:russn]]

I agree completely that it is more likely this is a gecko issue than a gaia issue. Fwiw, I was not able to reproduce on my flame with a recent 2.2 build:

BuidID: 20140908040204
Gaia: c71fd5d8c9c7cb021c97e5e9fbb29f92b50a084d
Gecko: 892768985915
Platform Version: 35.0a1

I believe I'm running JB, not exactly sure how to tell. From mozversion:
device_firmware_date: 1401721306
device_firmware_version_incremental: eng.cltbld.20140602.110131
device_firmware_version_release: 4.3

Comment 12

[Jason Smith [:jsmith]]

Okay - let's bounce this over to Eric's team then, since I think this is likely a core video/audio bug specific to KK.

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Russ Nicoletti [:russn] from comment #11)

> device_firmware_version_release: 4.3

Yep, 4.3 is JB. KK would be 4.4

Comment 14

[Eric Chou [:ericchou] [:echou]]

I'm bisecting and I think I'm very close to the root cause commit. Just like Jason and Russ' guess, it's more likely a Gecko issue. I tested with v2.1 and I fixed Gaia version to 

  6c17bb80bd2e33968a516c29849489995c54dd7b

and the video could be played normally when I used Gecko

  6d866d0479ba2bb75b14208350c176c87a4a4b36 (Aug 15)

on the other hand, video failed to be played when I used Gecko

  95deb9ffa819179f3572f24b363e288490315d77 (Aug 15)

I'll update more later.

Comment 15

[Eric Chou [:ericchou] [:echou]]

I think I found the regression is caused by patches of bug 1033902, therefore I'm going to assign this to Bruce.

Comment 16

[bhargavg1]

Can we try quickly toggling the property TM as and see if that helps

#adb root 
#adb shell "setprop audio.offload.disable 1"
#adb shell sync

Comment 17

[Eric Chou [:ericchou] [:echou]]

(In reply to bhargavg1 from comment #16)
> Can we try quickly toggling the property TM as and see if that helps
> 
> #adb root 
> #adb shell "setprop audio.offload.disable 1"
> #adb shell sync

Oh, it works. I have already told Bruce about this. Thanks.

Comment 18

[Bruce Sun [:brsun]]

(In reply to bhargavg1 from comment #9)
> (In reply to Russ Nicoletti [:russn] from comment #8)
> > bhargavg1, please give me access to the video clip on your google drive.
> > Thanks.
> 
> Done !

bhargavg1: Please give me the access as well. Thanks.

Comment 19

[bhargavg1]

(In reply to Bruce Sun [:brsun] from comment #18)
> (In reply to bhargavg1 from comment #9)
> > (In reply to Russ Nicoletti [:russn] from comment #8)
> > > bhargavg1, please give me access to the video clip on your google drive.
> > > Thanks.
> > 
> > Done !
> 
> bhargavg1: Please give me the access as well. Thanks.

Done !

The issue is with any aac clip or specifically I believe would be true for any TM supported formats

Comment 20

[Bruce Sun [:brsun]]

This is a regression bug while extracting the common logic of audio-offload-playback into the parent class. Originally the existence of the video track is examined in CheckAudioOffload() before the extraction, currently mInfo.mVideo.mHasVideo is examined instead. But since mInfo.mVideo.mHasVideo is updated after CheckAudioOffload() has been called, CheckAudioOffload() always treats |hasNoVideo| as false.

There might be two possible solutions:
 - 1. To examine the existence of the video track as before.
 - 2. To call CheckAudioOffload() later.

I prefer to adapt solution 2 because it doesn't need to expose track information into the parent class, and calling CheckAudioOffload() when ReadMetadata() is finished seems also reasonable.
But I would like to have feedback from the reviewer and the original author (bug 976172) first before doing such change.

Comment 21

[Bruce Sun [:brsun]]

Attachment: bug1064376_video_fails_if_audio_codec_offload_supported.solution_1.patch

Solution 1: To examine the existence of the video track as before.

Comment 22

[Bruce Sun [:brsun]]

Attachment: bug1064376_video_fails_if_audio_codec_offload_supported.solution_2.patch

Solution 2: To call CheckAudioOffload() later.

Comment 23

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #20)
> CheckAudioOffload() always treats |hasNoVideo| as false.

typo, correct one should be:

CheckAudioOffload() always treats |hasNoVideo| as true.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8487886 [details] [diff] [review]
bug1064376_video_fails_if_audio_codec_offload_supported.solution_2.patch

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

I prefer this one.

Comment 25

[Bruce Sun [:brsun]]

Comment on attachment 8487886 [details] [diff] [review]
bug1064376_video_fails_if_audio_codec_offload_supported.solution_2.patch

Request for review+/-.

Comment 26

[vasanth]

Comment on attachment 8487886 [details] [diff] [review]
bug1064376_video_fails_if_audio_codec_offload_supported.solution_2.patch

Calling CheckAudioOffload() just before ReadMetadata() finishes seems reasonable to me as well.

Comment 27

[Bruce Sun [:brsun]]

Attachment: bug1064376_video_fails_if_audio_codec_offload_supported.checkin.patch

TBPL results:
 - [default] media.omx.async.enabled pref-off: https://tbpl.mozilla.org/?tree=Try&rev=627a39738dd2
 - media.omx.async.enabled pref-on: https://tbpl.mozilla.org/?tree=Try&rev=8c0d8c1cfbf5

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21eb4f19d7fa

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/21eb4f19d7fa

Comment 30

[Bruce Sun [:brsun]]

Comment on attachment 8488594 [details] [diff] [review]
bug1064376_video_fails_if_audio_codec_offload_supported.checkin.patch

Approval Request Comment
[Feature/regressing bug #]:
 - 1033902
[User impact if declined]:
 - Video playback with black screen when the used audio codec is offload supported.
[Describe test coverage new/current, TBPL]:
 - [default] media.omx.async.enabled pref-off: https://tbpl.mozilla.org/?tree=Try&rev=627a39738dd2
 - media.omx.async.enabled pref-on: https://tbpl.mozilla.org/?tree=Try&rev=8c0d8c1cfbf5
[Risks and why]: 
 - Low.
 - JB and previous AOSP should not be affected by this patch because audio-offload-playback is a new feature only after KK.
 - Audio playback with any codec and video playback with offload-unsupported audio codec should not be affected by this patch because this is only for video playback with offload-supported audio codec.
 - Video playback with offload-supported audio codec should be ok by adapting this patch because the timing to check video playback is correct now.
[String/UUID change made/needed]:
 - N/A

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a9a2c33f2e1

Comment 32

[bhargavg1]

Bruce, quick question i believe the change was made when migrating IL codec to ACodec instead of OMXCodec. Wondering at this point was there any consideration in using the TunnelMode for Audio while video decode use case as well as that would save power

Comment 33

[bhargavg1]

(In reply to bhargavg1 from comment #32)
> Bruce, quick question i believe the change was made when migrating IL codec
> to ACodec instead of OMXCodec. Wondering at this point was there any
> consideration in using the TunnelMode for Audio while video decode use case
> as well as that would save power

will start a new for discussing this

Comment 34

[Bruce Sun [:brsun]]

(In reply to bhargavg1 from comment #33)
> will start a new for discussing this

take notes: bug 1068969

Comment 35

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame v2.1.MP4

This issue has been verified successfully on Flame 2.1 & 2.2.

Issue steps:
1.Add some clips in the test device.
2.Tap the clips in Video app to play.

Actual result:The clips are played without the black screen.


See attachment: Verify_Video_Flame v2.1.MP4
Reproducing rate: 0/10

Flame v2.1 version:
Gaia-Rev        8ae086c39011bc8842b2a19bb5267906fa22345a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebbd5c65c3c1
Build-ID        20141124094013
Version         34.0

Flame 2.2 version:
Gaia-Rev        aad40f6d6eb8f626c6a20db55b9f00d2e832f113
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/be4ba3d5ca9a
Build-ID        20141124100136
Version         36.0a1