1297307 - The video status keeps showing the loading symbol after moving the video process bar to the far right

Status: RESOLVED FIXED

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

Description

[Cynthia Tang [:cynthiatang]]

1. Launch Firefox (Fennec)
2. Enter "about:config" in URL address field 
3. Find "media.android-remote-codec.enabled" and enable it
4. New a tab
5. Go to youtube.com and play a video
6. Move the video progress bar to the far right


Actual Result:
- The video keeps showing the loading symbol

Expected Result:
- The video will be stop.

Comment 1

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

Cynthia,
I found if I do not do the 3rd step (not enable media.android-remote-codec.enabled), I still can see this problem. 
Could you double check it?

Comment 2

[Cynthia Tang [:cynthiatang]]

Hi Blake,
this issues cannot be reproduced after disabling "media.android-remote-codec.enabled". Thanks.

Device: Nexus 5
OS Version: Android 6.0.1
Firefox Version: Nightly 51.0a1 (2016-08-24)

Comment 3

[James Cheng[:JamesCheng]]

I can reproduce it on my NX5 easily when switching the pref.

I will start to debug and update as soon as I found something weird against the original local decoding module.

Comment 4

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

Just found that |RemoteVideoDecoder| [1] doesn't correctly handle EOS and |MediaDataDecoderCallback::DrainComplete()| is never called in [2].
I'd like to take this bug and will upload the patch later. Apology to James and many thanks for his time and efforts so far.

[1] http://searchfox.org/mozilla-central/source/dom/media/platforms/android/RemoteDataDecoder.cpp#114
[2] http://searchfox.org/mozilla-central/source/dom/media/platforms/android/RemoteDataDecoder.cpp#173

Comment 5

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

Change the priority from P1 to P2 since media.android-remote-codec.enabled is not default enabled yet.

Comment 6

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

Attachment: Bug 1297307 - Add extra video duration element for EOS sample.

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

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8786620 [details]
Bug 1297307 - Add extra video duration element for EOS sample.

https://reviewboard.mozilla.org/r/75548/#review73576

::: dom/media/platforms/android/RemoteDataDecoder.cpp:152
(Diff revision 1)
>        ok |= NS_SUCCEEDED(aInfo->Size(&size));
>        MOZ_ASSERT(ok);
>  
>        NS_ENSURE_TRUE_VOID(ok);
>  
> -      if (size > 0) {
> +      if (size > 0 && durationUs.value() > 0) {

Looking at MediaCodecDataDecoder, we simply don't call ProcessOutput() (which is similar to HandleOutput) for EOS samples. Can we do something similar here?

Comment 8

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

Comment on attachment 8786620 [details]
Bug 1297307 - Add extra video duration element for EOS sample.

https://reviewboard.mozilla.org/r/75548/#review73576

> Looking at MediaCodecDataDecoder, we simply don't call ProcessOutput() (which is similar to HandleOutput) for EOS samples. Can we do something similar here?

Sometimes a decoder attaches EOS flag to its final output instead of emitting empty EOS buffer [1]. Those samples will be mssing if not calling MediaDataDecoderCallback::Output().

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1207214#c15

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

I don't think we've ever seen an EOS flag in the AndroidDecoderModule before unless we set that flag on the input sample ourselves (during drain, for example). Can you elaborate on how this occurs in RemoteDataDecoder? The decoder just gets samples and spits stuff out, how would it know whether or not it's EOS on its own?

Comment 10

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> I don't think we've ever seen an EOS flag in the AndroidDecoderModule before
> unless we set that flag on the input sample ourselves (during drain, for
> example). Can you elaborate on how this occurs in RemoteDataDecoder? The
> decoder just gets samples and spits stuff out, how would it know whether or
> not it's EOS on its own?

 Like MediaCodecDataDecoder, RemoteDataDecoder sends an empty sample with EOS flag set in Drain(). In response to the EOS input, the decoder will spit an output buffer with EOS flag set to indicate that it has no further output.
 What I meant in comment 8 is that while there are zero length output buffers with EOS flag set, some  decoder implementation returns non-zero length EOS output buffer with legitimate contents that should be processed/handled. Perhaps the author thought saving the caller one dequeueOutputBuffer() invocation by combining the final output buffer and EOS is a good idea?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to John Lin [:jolin][:jhlin] from comment #10)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> > I don't think we've ever seen an EOS flag in the AndroidDecoderModule before
> > unless we set that flag on the input sample ourselves (during drain, for
> > example). Can you elaborate on how this occurs in RemoteDataDecoder? The
> > decoder just gets samples and spits stuff out, how would it know whether or
> > not it's EOS on its own?
> 
>  Like MediaCodecDataDecoder, RemoteDataDecoder sends an empty sample with
> EOS flag set in Drain(). In response to the EOS input, the decoder will spit
> an output buffer with EOS flag set to indicate that it has no further output.
>  What I meant in comment 8 is that while there are zero length output
> buffers with EOS flag set, some  decoder implementation returns non-zero
> length EOS output buffer with legitimate contents that should be
> processed/handled. Perhaps the author thought saving the caller one
> dequeueOutputBuffer() invocation by combining the final output buffer and
> EOS is a good idea?

Ah, I see. Thanks for the clarification. I still kinda think adding a 0 duration is pretty hacky, but I see the reason now.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8786620 [details]
Bug 1297307 - Add extra video duration element for EOS sample.

https://reviewboard.mozilla.org/r/75548/#review75068

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/847eab1caa7c
Add extra video duration element for EOS sample. r=snorp

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/847eab1caa7c