Closed
Bug 1197319
Opened 9 years ago
Closed 9 years ago
Enable ffvp9 support on Linux for WebM decoding
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ajones, Assigned: ajones)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
The ffvp9 decoder is nearly twice as fast on x86 compared to libvpx when used in multi-threading mode. We have most of the plumbing in place on Linux so it would be worth enabling it.
https://drive.google.com/file/d/0B9stcb8aInKAM2xhSktiVkU0VEk/view
Assignee | ||
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ajones
Comment 1•9 years ago
|
||
Attachment #8658046 -
Flags: review?(ajones)
Comment 2•9 years ago
|
||
Attachment #8658047 -
Flags: review?(ajones)
Comment 3•9 years ago
|
||
Interestingly:
The 4K sample here:
http://www.reduser.net/forum/showthread.php?111230-Google-VP9-4K-HD-Sample
Plays super nicely with ffmpeg 2.8 ; however using the default libvpx it plays super slow in Nightly, but plays well in 42. (file only plays for about 3s with the old WebMReader).
so there's a regression in current nightly.
Comment 4•9 years ago
|
||
Jan, there's been no modifications in the VPXDecoder between 42 and 43.
would you have an idea on what could be the cause of the regression in nightly for that file ?
Flags: needinfo?(j)
Assignee | ||
Updated•9 years ago
|
Attachment #8658047 -
Flags: review?(ajones) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8658046 -
Flags: review?(ajones) → review+
Comment 5•9 years ago
|
||
Attachment #8658491 -
Flags: review?(ajones)
Updated•9 years ago
|
Attachment #8658047 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8658491 -
Flags: review?(ajones) → review+
Comment 6•9 years ago
|
||
A VP9 or VP9 packet may contains alternate frames. They need to be fed separately to the ffmpeg decoder.
Attachment #8658586 -
Flags: review?(ajones)
Comment 7•9 years ago
|
||
ffvpx provides *much* better performance on my machine (mac pro 8 cores).
(all done on a debug build)
With libvpx, 1080p plays with about 18% frame drop.
4K video current do not decode properly, having an effective frame rate of about 4fps.
With the ffmpeg VP9 decoder, 4K videos plays perfectly, no frame drops whatsoever...
Updated•9 years ago
|
Comment 8•9 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Jan, there's been no modifications in the VPXDecoder between 42 and 43.
>
> would you have an idea on what could be the cause of the regression in
> nightly for that file ?
Not aware of any regression that could trigger this,
nightly and aurora builds seam to have about the same speed here.
Flags: needinfo?(j)
Comment 9•9 years ago
|
||
I'm fairly convinced that the issue is due to the WebMDemuxer not handling properly the skip to next keyframe logic. It will just never return the proper next keyframe position , so we'll never skip frames. That would explain why playback appears so slow
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8658586 [details] [diff] [review]
P3. Parse raw data to identify single frames before decoding.
Review of attachment 8658586 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/platforms/ffmpeg/FFmpegH264Decoder.cpp
@@ +71,5 @@
> FFmpegH264Decoder<LIBAV_VER>::DecodeResult
> FFmpegH264Decoder<LIBAV_VER>::DoDecodeFrame(MediaRawData* aSample)
> {
> + uint8_t* in_data = const_cast<uint8_t*>(aSample->Data());
> + size_t in_size = aSample->Size();
These variables are the wrong case convention.
@@ +88,5 @@
> + }
> + bool gotFrame = false;
> + while (in_size) {
> + uint8_t* data;
> + int size;
Should this be size_t?
@@ +94,5 @@
> + in_data, in_size,
> + aSample->mTime, aSample->mTimecode,
> + aSample->mOffset);
> + in_data += len;
> + in_size -= len;
We should sanity check that len >= in_size.
@@ +108,5 @@
> + }
> + }
> + }
> + return gotFrame ? DecodeResult::DECODE_FRAME : DecodeResult::DECODE_NO_FRAME;
> + } else {
This else clause is redundant because of the return above.
Attachment #8658586 -
Flags: review?(ajones) → review-
Comment 11•9 years ago
|
||
Comment on attachment 8658586 [details] [diff] [review]
P3. Parse raw data to identify single frames before decoding.
Review of attachment 8658586 [details] [diff] [review]:
-----------------------------------------------------------------
r- for a variable name issue ? :)
::: dom/media/platforms/ffmpeg/FFmpegH264Decoder.cpp
@@ +71,5 @@
> FFmpegH264Decoder<LIBAV_VER>::DecodeResult
> FFmpegH264Decoder<LIBAV_VER>::DoDecodeFrame(MediaRawData* aSample)
> {
> + uint8_t* in_data = const_cast<uint8_t*>(aSample->Data());
> + size_t in_size = aSample->Size();
this is from ffmpeg sample code:
http://www.ffmpeg.org/doxygen/2.0/group__lavc__parsing.html#ga691ca0258e91f99297e7726f56d8c247
I didn't know we had a case convention for local variable either :)
@@ +88,5 @@
> + }
> + bool gotFrame = false;
> + while (in_size) {
> + uint8_t* data;
> + int size;
no; av_parser_parse2 takes an int* ; size_t will be 64 bits on 64 bits OS
@@ +108,5 @@
> + }
> + }
> + }
> + return gotFrame ? DecodeResult::DECODE_FRAME : DecodeResult::DECODE_NO_FRAME;
> + } else {
okay
Comment 12•9 years ago
|
||
A VP9 or VP9 packet may contains alternate frames. They need to be fed separately to the ffmpeg decoder.
Attachment #8659021 -
Flags: review?(ajones)
Updated•9 years ago
|
Attachment #8658586 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8659021 -
Flags: review?(ajones) → review+
Comment 13•9 years ago
|
||
ffmpeg isn't enabled but just in case https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a27053de697
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/47fd59525c65
https://hg.mozilla.org/mozilla-central/rev/dec6b701c9ea
https://hg.mozilla.org/mozilla-central/rev/3a5f6d921972
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 16•9 years ago
|
||
Sorry for Bugspam, but is this enabled by "media.fragmented-mp4.ffmpeg.enabled" true?
(Seems so for me, 4k video mentioned above works fine/way better with this)
If yes, wouldn't that mean, that ffmpeg is now (with above setting) responsible for h.264 too?
So, should h.264 gstreamer hardware-accel. work one day (and ffmpeg h.264 hardware accel. not), one could not have both: better h.264 via hardware gstreamer accel. and better ffvp9 (instead of slower libvpx)?
Setting handler per codec would be better imho.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Stebs from comment #16)
> So, should h.264 gstreamer hardware-accel. work one day (and ffmpeg h.264
> hardware accel. not), one could not have both: better h.264 via hardware
> gstreamer accel. and better ffvp9 (instead of slower libvpx)?
No. We are not doing that.
You need to log in
before you can comment on or make changes to this bug.
Description
•