1299068 - The video is choppy when playing a video from Vimeo

Status: VERIFIED 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 Go to vimeo.com 
6. Play any video on the Vimeo home page.

Actual Result:
- The video is choppy.

Expected Result:
- Videos could be played clearly. Everything is smooth.

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

Comment 1

[James Cheng[:JamesCheng]]

I think those choppy or playing video not smoothly issues are related to repeatedly memory copy by current code design. Set the dependency to bug 1295106 which intends to solve performance issue by shared memory.

Comment 2

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

After comparing systrace before and after enabling remote codec, I think I found the root cause of this problem.
Looks like that even though the timestamps in SurfaceTexture images [1] are correctly specified by VideoSink, and compositor calls SurfaceTexture.updateTexImage() at the right time, SurfaceTexture only updates its content with the most recent image from video decoder (i.e., the one provided by latest MediaCodec.releaseOutputBuffer(..., true)). This requires video decoder to output in the same frequency as the frame rate, or some frames are going to be dropped.
Current implementation of RemoteCodec outputs ASAP and looks more like bursts than pulses, so only the last one in a burst is shown.
I am surprised that MediaCodecDataDecoder can actually fulfill this requirement because there seems no code explicitly making it happen. Will check how it achieves that.

[1] http://searchfox.org/mozilla-central/source/gfx/layers/composite/CompositableHost.h#192
[2] http://searchfox.org/mozilla-central/source/gfx/gl/GLBlitHelper.cpp#701

Comment 3

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

As mentioned in comment 2, MediaCodecDataDecoder seems drop much less frames. It's because MediaCodecDataDecoder::DecoderLoop()[1] runs as following:

 DecoderLoop() +--+> wait for input +--> queue sample +--> dequeue output +-+
                  |                                                         |
                  +---------------------------------------------------------+

 When decoding, the outputs are throttled by inputs, which come in the same rate as VideoSink consumes outputs.

 [1] http://searchfox.org/mozilla-central/source/dom/media/platforms/android/MediaCodecDataDecoder.cpp#610
 [2] http://searchfox.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1107

Comment 4

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

Dropping frames or not, the compositor doesn't show video frames correctly on Android. In current implementation, MediaCodecDataDecoder updates the surface before calling MediaDataDecoderCallback::InputExhausted() [1], so the contents of last frame in VideoQueue is presented.

On Android, we need to notify data decoder when video sink sends frames to compositor. With that and making video sink send only one frame at a time, the surface will be updated right before compositor renders the frame.

 [1] http://searchfox.org/mozilla-central/source/dom/media/platforms/android/MediaCodecDataDecoder.cpp#603

Comment 5

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

Attachment: Bug 1299068 - part 1: code refactoring/clean-up

- move all buffer related code from the Callbacks class to (Input|Output)Pocessor
- don't implicitly release output buffer to codec. Do it when client calls releaseOutput()
- fix buffer management problem in reset()
- minor code formatting issue

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

Comment 6

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

Attachment: bug 1299068 - part 2: add parameter to IPC method for rendering output or not.

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

Comment 7

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

Attachment: Bug 1299068 - part 3: notify when VideoData are sent to compositor.

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

Comment 8

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

Attachment: Bug 1299068 - part 4: on Android, send one frame to compositor at a time.

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

Comment 9

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

Attachment: Bug 1299068 - part 5: release/render buffers when VideoData sent to compositor.

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

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8816001 [details]
Bug 1299068 - part 4: on Android, send one frame to compositor at a time.

https://reviewboard.mozilla.org/r/96766/#review97018

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97028

::: dom/media/MediaData.cpp:146
(Diff revision 1)
>  
> +void
> +VideoData::MarkSentToCompositor()
> +{
> +    mSentToCompositor = true;
> +    if (mListener != nullptr) {

This should be a one-off listener, right?

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97030

::: dom/media/mediasink/VideoSink.cpp:362
(Diff revision 1)
>    TimeStamp lastFrameTime;
>    MediaSink::PlaybackParams params = mAudioSink->GetPlaybackParams();
>    for (uint32_t i = 0; i < frames.Length(); ++i) {
>      VideoData* frame = frames[i]->As<VideoData>();
>  
> -    frame->mSentToCompositor = true;
> +    frame->MarkSentToCompositor();

The improvement here may depend on how fast the video sample is decoded.
Several VideoData samples may be marked "mSentToComposistor=true" before they're actually composited.
If the decoding time is close to the interval of gecko composition, video playback should be smooth, if not, it may still become a bit choppy, right ?

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97030

> The improvement here may depend on how fast the video sample is decoded.
> Several VideoData samples may be marked "mSentToComposistor=true" before they're actually composited.
> If the decoding time is close to the interval of gecko composition, video playback should be smooth, if not, it may still become a bit choppy, right ?

oh, I saw the pref you've modified. "media.video-queue.send-to-compositor-size = 1" should get this work ! cool

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97030

> oh, I saw the pref you've modified. "media.video-queue.send-to-compositor-size = 1" should get this work ! cool

Even with "media.video-queue.send-to-compositor-size = 1", the listener is notifed before the image is actually composed. Isn't it we don't want to release the buffer until it is done with the compositor?

Comment 15

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

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97030

> Even with "media.video-queue.send-to-compositor-size = 1", the listener is notifed before the image is actually composed. Isn't it we don't want to release the buffer until it is done with the compositor?

The name is misleading. :(
In this case, the action of 'releasing' is actually 'rendering the content of this buffer to surface' -- https://developer.android.com/reference/android/media/MediaCodec.html#releaseOutputBuffer(int,%20boolean)

Comment 16

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

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97028

> This should be a one-off listener, right?

Yes. Will address that in next version. Do you prefer clearing the `mListener` ref, or early return if `mSentToCompositor` is `true`?

Comment 17

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

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97044

Comment 18

[Parker]

can you specify what specific version you're having this issue on?

Comment 19

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

(In reply to Parker from comment #18)
> can you specify what specific version you're having this issue on?

 It can be reproduced on Fennec Nightly.

Comment 20

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

Comment on attachment 8815998 [details]
Bug 1299068 - part 1: code refactoring/clean-up

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

Comment 21

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

Comment on attachment 8815999 [details]
bug 1299068 - part 2: add parameter to IPC method for rendering output or not.

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

Comment 22

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

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

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

Comment 23

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

Comment on attachment 8816001 [details]
Bug 1299068 - part 4: on Android, send one frame to compositor at a time.

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

Comment 24

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

Comment on attachment 8816002 [details]
Bug 1299068 - part 5: release/render buffers when VideoData sent to compositor.

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

Comment 25

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

Comment on attachment 8815998 [details]
Bug 1299068 - part 1: code refactoring/clean-up

https://reviewboard.mozilla.org/r/96760/#review97498

Comment 26

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

Comment on attachment 8815999 [details]
bug 1299068 - part 2: add parameter to IPC method for rendering output or not.

https://reviewboard.mozilla.org/r/96762/#review97500

Comment 27

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

Comment on attachment 8816002 [details]
Bug 1299068 - part 5: release/render buffers when VideoData sent to compositor.

https://reviewboard.mozilla.org/r/96768/#review97502

Nice. This is something I thought about before, but hadn't ever really figured out. Clever.

Comment 28

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97968

::: dom/media/MediaData.h:452
(Diff revision 2)
>      YUVColorSpace mYUVColorSpace = YUVColorSpace::BT601;
>    };
>  
> +  class Listener {
> +  public:
> +    virtual void OnSendToCompositor() = 0;

OnSentToCompositor

::: dom/media/MediaData.h:555
(Diff revision 2)
> +
>  protected:
>    ~VideoData();
> +
> +  bool mSentToCompositor;
> +  UniquePtr<Listener> mListener;

indention.

::: dom/media/MediaData.cpp:159
(Diff revision 2)
> +    return;
> +  }
> +
> +  mSentToCompositor = true;
> +  if (mListener != nullptr) {
> +    mListener->OnSendToCompositor();

Clear mListener so we don't keep it alive longer than needed.

Comment 29

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

Comment on attachment 8815998 [details]
Bug 1299068 - part 1: code refactoring/clean-up

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

Comment 30

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

Comment on attachment 8815999 [details]
bug 1299068 - part 2: add parameter to IPC method for rendering output or not.

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

Comment 31

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

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

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

Comment 32

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

Comment on attachment 8816001 [details]
Bug 1299068 - part 4: on Android, send one frame to compositor at a time.

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

Comment 33

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

Comment on attachment 8816002 [details]
Bug 1299068 - part 5: release/render buffers when VideoData sent to compositor.

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

Comment 34

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

Comment on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97968

> indention.

It's fine in my diff file. Could it be some bug of Reviewboard?
Anyway, v3 doesn't have the problem.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c871270d176e
part 1: code refactoring/clean-up r=snorp
https://hg.mozilla.org/integration/autoland/rev/956f17ac2522
part 2: add parameter to IPC method for rendering output or not. r=snorp
https://hg.mozilla.org/integration/autoland/rev/cd931b83e499
part 3: notify when VideoData are sent to compositor. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f417fea060f7
part 4: on Android, send one frame to compositor at a time. r=jya
https://hg.mozilla.org/integration/autoland/rev/8282bb09dc78
part 5: release/render buffers when VideoData sent to compositor. r=snorp

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c871270d176e
https://hg.mozilla.org/mozilla-central/rev/956f17ac2522
https://hg.mozilla.org/mozilla-central/rev/cd931b83e499
https://hg.mozilla.org/mozilla-central/rev/f417fea060f7
https://hg.mozilla.org/mozilla-central/rev/8282bb09dc78

Comment 37

[Carmen Fat [:cfat] - Ecosystem QA]

The fix was verified on latest Nightly build 53.0a1 (2016-12-11) and the videos are playing smooth.

Tested with:
- Nexus 9 (Android 7.0);
- Honor 5x (Android 5.1.1).

I'm marking this bug as Verified since the issue is no longer reproducing.

Comment 38

[Jean-Yves Avenard [:jya]]

could you please test with older android version (inclusive of 4.0). thank you

Comment 39

[Carmen Fat [:cfat] - Ecosystem QA]

Hello, 

The oldest Android versions I have are the next ones:
- Android 4.4.2 (Device: Lenovo A536);
- Android 4.4.4 (Device: Motorola XT910).

Following the STR provided in the Description results in Firefox crashing.
Here is one of the crash reports:
https://crash-stats.mozilla.com/report/index/a2ef590b-bf4b-4471-8c98-b34582161212

Comment 40

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

(In reply to Carmen Fat from comment #39)
> Hello, 
> 
> The oldest Android versions I have are the next ones:
> - Android 4.4.2 (Device: Lenovo A536);
> - Android 4.4.4 (Device: Motorola XT910).
> 
> Following the STR provided in the Description results in Firefox crashing.
> Here is one of the crash reports:
> https://crash-stats.mozilla.com/report/index/a2ef590b-bf4b-4471-8c98-
> b34582161212

 Could you please upload logcat output of the crash? I've tried STR on Nexus 5/Android 4.4 but couldn't reproduce the crash. Thanks a lot.

Comment 41

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: Crash logcat.txt

Comment 42

[Carmen Fat [:cfat] - Ecosystem QA]

Hello, 

This was the first time I made a logcat, so I'm sorry if it's too long but I wasn't sure enough which part of it you'll be needing.

Comment 43

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

(In reply to Carmen Fat from comment #42)
> Hello, 
> 
> This was the first time I made a logcat, so I'm sorry if it's too long but I
> wasn't sure enough which part of it you'll be needing.

 Carmen, thank you for the help.
 It seems the uploaded log was filtered and contains only those of 'org.mozilla.fennec' process. Unfortunately we also need log of 'org.mozilla.fennec:media'. Could you please record it one more time with 'All messages (no filters)' selected? Thanks again!

 BTW IMHO, for debugging, usually the longer the log, the better chance we will find the cause. :)

Comment 44

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: Crash log - no filters.txt

Comment 45

[Carmen Fat [:cfat] - Ecosystem QA]

Hello again, 

I recorded the log with no filters this time :)

Comment 46

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

(In reply to Carmen Fat from comment #45)
> Hello again, 
> 
> I recorded the log with no filters this time :)

Thanks a lot Carmen, I found a possible cause of the exception in your latest log and filed bug 1323687 for it.