bhargavg1 Reporter
	Description • 10 years ago

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

	bhargavg1 Reporter
	Comment 1 • 10 years ago

[Blocking Requested - why for this release]:

	bhargavg1 Reporter
	Comment 2 • 10 years ago

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

	Jason Smith [:jsmith]
	Comment 3 • 10 years ago

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

	Chris Kreinbring [:CKreinbring]
	Comment 4 • 10 years ago

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

	Hema Koka [:hema]
	Comment 5 • 10 years ago

Russ will take an initial look. 

Blocking for regression.

	Chris Kreinbring [:CKreinbring]
	Comment 6 • 10 years ago

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

	Joshua Mitchell (Inactive)
	Comment 7 • 10 years ago

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.

	Russ Nicoletti [:russn]
	Comment 8 • 10 years ago

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

	bhargavg1 Reporter
	Comment 9 • 10 years ago

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

Done !

	Jason Smith [:jsmith]
	Comment 10 • 10 years ago

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?

	Russ Nicoletti [:russn]
	Comment 11 • 10 years ago

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

	Jason Smith [:jsmith]
	Comment 12 • 10 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 13 • 10 years ago

(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

	Eric Chou [:ericchou] [:echou]
	Comment 14 • 10 years ago

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.

	Eric Chou [:ericchou] [:echou]
	Comment 15 • 10 years ago

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

	bhargavg1 Reporter
	Comment 16 • 10 years ago

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

	Eric Chou [:ericchou] [:echou]
	Comment 17 • 10 years ago

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

	Bruce Sun [:brsun] Assignee
	Comment 18 • 10 years ago

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

	bhargavg1 Reporter
	Comment 19 • 10 years ago

(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

	Bruce Sun [:brsun] Assignee
	Comment 20 • 10 years ago

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.

	Bruce Sun [:brsun] Assignee
	Comment 21 • 10 years ago

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

	Bruce Sun [:brsun] Assignee
	Comment 22 • 10 years ago

Solution 2: To call CheckAudioOffload() later.

	Bruce Sun [:brsun] Assignee
	Comment 23 • 10 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 10 years ago

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.

	Bruce Sun [:brsun] Assignee
	Comment 25 • 10 years ago

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

Request for review+/-.

	vasanth
	Comment 26 • 10 years ago

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.

	Bruce Sun [:brsun] Assignee
	Comment 27 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

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

	Phil Ringnalda (:philor)
	Comment 29 • 10 years ago

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

	Bruce Sun [:brsun] Assignee
	Comment 30 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 10 years ago

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

	bhargavg1 Reporter
	Comment 32 • 10 years ago

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

	bhargavg1 Reporter
	Comment 33 • 10 years ago

(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

	Bruce Sun [:brsun] Assignee
	Comment 34 • 10 years ago

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

take notes: bug 1068969

	Lancy(Leave from Mozilla)
	Comment 35 • 10 years ago

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