1363276 - Android remote decoder doesn't return video frames on Android 4.2.2

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[John Lin [:jhlin][:jolin]]

The size in the buffer info returned by 4.2.2 video decoder is 0 and the buffer will be dropped in OnOutput.

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1363276 - discard video output buffers according to presentation time rather than size.

Review commit: https://reviewboard.mozilla.org/r/137396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137396/

Comment 2

[Eugen Sawin [:esawin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

https://reviewboard.mozilla.org/r/137396/#review140590

What about the audio decoder, can this happen there, too?
Is it even save to decode with zero-sized buffers, isn't that a bug in the codec implementation?

::: dom/media/platforms/android/RemoteDataDecoder.cpp:135
(Diff revision 1)
>        if (!mDecoder->mInputInfos.Find(presentationTimeUs, inputInfo)
>            && !isEOS) {
>          return;
>        }
>  
> -      if (size > 0) {
> +      if (presentationTimeUs >= 0) {

size is now unused and can be removed.

Comment 3

[Eugen Sawin [:esawin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

https://reviewboard.mozilla.org/r/137396/#review140590

s/save/safe

Comment 4

[John Lin [:jhlin][:jolin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137396/diff/1-2/

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

https://reviewboard.mozilla.org/r/137396/#review140590

This only applies to video for it uses surface/graphic buffer as output target.
ACodec fixed it after 4.2.2: https://android.googlesource.com/platform/frameworks/av/+/21ad778dcfcddb8f8fd9dc3fe4992fbef246c511

Comment 6

[John Lin [:jhlin][:jolin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137396/diff/2-3/

Comment 7

[John Lin [:jhlin][:jolin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137396/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eb6bcebaf3d
discard video output buffers according to presentation time rather than size. r=esawin

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0eb6bcebaf3d

Comment 10

[Blake Wu [:bwu][:blakewu]]

Ioana,
Can you help test it with the latest nightly?

Comment 11

[Gerry Chang [:gchang]]

Track 54+ as OOP is enabled in 54.

Comment 12

[Paul Yang [:pyang] (away)]

Tested by both latest nightly and beta.  Using Google Nexus4 by Android 4.2.2

STR: 
1. open browser
2. navigate to youtube
3. choose random video, play, pause, stop, reload for several times.
4. tap another video by recommended list.
5. repeat step 3
6. test other video source, like facebook.

EXPECTED RESULT:
- video can be played, paused, reloaded successfully.

ACTUAL RESULT:
beta
- none of youtube video can be played; only placeholder(spinning icon) was showed.
- facebook video can't be played.
nightly
- complete STR can be performed successfully.

Comment 13

[John Lin [:jhlin][:jolin]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: video fails to play on Android 4.2.2 devices
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes. see comment 12
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: presentation time is more reliable than size
[String changes made/needed]: none

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

https://reviewboard.mozilla.org/r/137396/#review140590

> size is now unused and can be removed.

it is also possible for presentationTimeUs to be negative... Should set a particular value (like INT64_MIN) rather than dropping all frames with a negative timestamp.

Comment 15

[John Lin [:jhlin][:jolin]]

Will open another bug to handle negative timestamp frames.

Comment 16

[John Lin [:jhlin][:jolin]]

Bug 1366706 was filed as follow-up of comment 14.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8865772 [details]
Bug 1363276 - discard video output buffers according to presentation time rather than size.

This fix a critical issue that video fails to play on Android 4.2.2 devices and was verified. Beta54+. Should be in 54 beta 10.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8349864eb2bb

Comment 19

[Ioana Chiorean]

Prestigio PSP5508DUO Android 4.4.2

STR: 
1. open browser
2. navigate to youtube,facebook, tiff.ro etc.
3. choose random video, play, pause, stop, reload for several times.

Expected and Actual Results:
- media process is showed separately
- media is played correctly

Comment 20

[Stefan Deiac]

Remove qe-verify based on comment 19.