Closed
Bug 1299068
Opened 8 years ago
Closed 8 years ago
The video is choppy when playing a video from Vimeo
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P2)
Firefox for Android Graveyard
Audio/Video
Tracking
(platform-rel -, firefox53 verified)
VERIFIED
FIXED
Firefox 53
People
(Reporter: cynthiatang, Assigned: jhlin)
References
Details
(Whiteboard: [platform-rel-Vimeo])
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
snorp
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
snorp
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jwwang
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jya
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
snorp
:
review+
|
Details |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details |
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)
Updated•8 years ago
|
Priority: -- → P2
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
platform-rel: --- → ?
Whiteboard: [platform-rel-Vimeo]
Updated•8 years ago
|
platform-rel: ? → -
Assignee | ||
Comment 2•8 years ago
|
||
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
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
mozreview-review |
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 11•8 years ago
|
||
mozreview-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 12•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review-reply |
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?
Assignee | ||
Comment 15•8 years ago
|
||
mozreview-review-reply |
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)
Assignee | ||
Comment 16•8 years ago
|
||
mozreview-review-reply |
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`?
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
can you specify what specific version you're having this issue on?
Flags: needinfo?(ctang)
Assignee | ||
Comment 19•8 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•8 years ago
|
||
mozreview-review |
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 26•8 years ago
|
||
mozreview-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 27•8 years ago
|
||
mozreview-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+
Updated•8 years ago
|
Assignee: nobody → jolin
Comment 28•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
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
Comment 36•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 37•8 years ago
|
||
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
Comment 38•8 years ago
|
||
could you please test with older android version (inclusive of 4.0). thank you
Comment 39•8 years ago
|
||
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
Assignee | ||
Comment 40•8 years ago
|
||
(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)
Comment 41•8 years ago
|
||
Flags: needinfo?(carmen.fat)
Comment 42•8 years ago
|
||
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.
Assignee | ||
Comment 43•8 years ago
|
||
(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 45•8 years ago
|
||
Hello again,
I recorded the log with no filters this time :)
Assignee | ||
Comment 46•8 years ago
|
||
(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.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•