Closed Bug 1299068 Opened 3 years ago Closed 3 years ago

The video is choppy when playing a video from Vimeo

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
platform-rel --- -
firefox53 --- verified

People

(Reporter: cynthiatang, Assigned: jhlin)

References

Details

(Whiteboard: [platform-rel-Vimeo])

Attachments

(7 files)

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)
Priority: -- → P2
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.
Depends on: 1295106
platform-rel: --- → ?
Whiteboard: [platform-rel-Vimeo]
platform-rel: ? → -
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
Blocks: 1311960
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
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 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
Attachment #8816001 - Flags: review?(jyavenard) → review+
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 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 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 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 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 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 on attachment 8816000 [details]
Bug 1299068 - part 3: notify when VideoData are sent to compositor.

https://reviewboard.mozilla.org/r/96764/#review97044
can you specify what specific version you're having this issue on?
Flags: needinfo?(ctang)
(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.
Flags: needinfo?(ctang)
Comment on attachment 8815998 [details]
Bug 1299068 - part 1: code refactoring/clean-up

https://reviewboard.mozilla.org/r/96760/#review97498
Attachment #8815998 - Flags: review?(snorp) → review+
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
Attachment #8815999 - Flags: review?(snorp) → review+
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.
Attachment #8816002 - Flags: review?(snorp) → review+
Assignee: nobody → jolin
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.
Attachment #8816000 - Flags: review?(jwwang) → review+
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.
Pushed by jolin@mozilla.com:
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
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.
Status: RESOLVED → VERIFIED
could you please test with older android version (inclusive of 4.0). thank you
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
(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.
Flags: needinfo?(carmen.fat)
Attached file Crash logcat.txt
Flags: needinfo?(carmen.fat)
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.
(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. :)
Hello again, 

I recorded the log with no filters this time :)
(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.
You need to log in before you can comment on or make changes to this bug.