Bug 1240412 (vp9-in-mp4)

Add VP9-in-MP4 support to Rust demuxer

RESOLVED FIXED in Firefox 51

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: ayang)

Tracking

({feature})

Trunk
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox46 affected, firefox51 fixed)

Details

()

Attachments

(2 attachments)

No description provided.
Alias: vp9-in-mp4
Keywords: feature
No longer depends on: 1238420
P2 because VP9-in-MP4 is important, but not a release blocker.
Priority: -- → P2
We have mp4parse-rust reading vp9 Sample Entries as of 0.2.0.
Depends on: 1243234
Chromium landed support for VP9 in MP4 yesterday (pref'd off):

https://bugs.chromium.org/p/chromium/issues/detail?id=580623
Wow, that's great!
Mass change P2 -> P3
Priority: P2 → P3
Matthew, are you still able to work on this?
Assignee: giles → kinetik
Assignee: kinetik → alwu
Reassign to the correct person. :-)
Assignee: alwu → ayang
Sorry, Alfredo and Alastor! I was too quick with the auto-complete.
The test video is from test sample in chromium.
ffvpx fails to decode the vp9 sample in https://github.com/Netflix/vp9-dash/tree/master/DASH-Samples.
libvpx is ok.
(In reply to Alfredo Yang (:alfredo) from comment #13)
> ffvpx fails to decode the vp9 sample in
> https://github.com/Netflix/vp9-dash/tree/master/DASH-Samples.
> libvpx is ok.

I think this is because ffvpx can't handle superframes but libvpx can.  That's solved/worked around internally in ffmpeg by enabling the bitstream header parser when demuxing (e.g. in Matroska here https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskadec.c#L2292, and I have a patch for their MP4 demuxer I need to upstream).
Comment on attachment 8786194 [details]
Bug 1240412 - add VP9-in-MP4 support to Rust demuxer.

https://reviewboard.mozilla.org/r/74914/#review73758

Looks fine. This will have to wait until `mp4parse_is_fragmented()` lands. Once the upstream PR is merged I'll file a bug to sync the copy in m-c.

::: dom/media/platforms/agnostic/VPXDecoder.cpp:32
(Diff revision 1)
>    if (aMimeType.EqualsLiteral("video/webm; codecs=vp8")) {
>      return VPXDecoder::Codec::VP8;
>    } else if (aMimeType.EqualsLiteral("video/webm; codecs=vp9")) {
>      return VPXDecoder::Codec::VP9;
> +  } else if (aMimeType.EqualsLiteral("video/vp9")) {
> +    return VPXDecoder::Codec::VP9;

This should probably be "video/mp4; codecs=vp9" but it's wrong for opus too, so that should be a separate bug. "video/vp9" is appropriate for the demuxed content if the caller doesn't actually care about the container. I guess WebRTC doesn't use this decoder.
Attachment #8786194 - Flags: review?(giles) → review+
As Vimeo is looking into VP9, I really hope this feature sees the light (and enabled by default unlike in Chrome).
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c82d3d5d0886
add VP9-in-MP4 support to Rust demuxer. r=rillian
https://hg.mozilla.org/mozilla-central/rev/c82d3d5d0886
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8786194 [details]
Bug 1240412 - add VP9-in-MP4 support to Rust demuxer.

https://reviewboard.mozilla.org/r/74914/#review76226

::: dom/media/platforms/agnostic/VPXDecoder.cpp:237
(Diff revision 2)
>    return ((aCodecMask & VPXDecoder::VP8) &&
>            aMimeType.EqualsLiteral("video/webm; codecs=vp8")) ||
>           ((aCodecMask & VPXDecoder::VP9) &&
> -          aMimeType.EqualsLiteral("video/webm; codecs=vp9"));
> +          aMimeType.EqualsLiteral("video/webm; codecs=vp9")) ||
> +         ((aCodecMask & VPXDecoder::VP9) &&
> +          aMimeType.EqualsLiteral("video/vp9"));

should have used the same codec type, and not introduce a new one...

Please open a follow up bug.

using video/vp9 everywhere for vp9 and video/vp8 for vp8

I can't find official reference to what vP[8-9] codec mimetype is.
Comment on attachment 8786194 [details]
Bug 1240412 - add VP9-in-MP4 support to Rust demuxer.

https://reviewboard.mozilla.org/r/74914/#review76228

The VPX decoder is no longer use for VP9/VP8 playback.

So this change will NOT allow to decode vp9 in mp4. You need to modify the FFVPXDecodeModule
Flags: needinfo?(ayang)
Alfredo, is this something manual QA could help test?
Flags: qe-verify?
(In reply to Jean-Yves Avenard [:jya] from comment #21)
> Comment on attachment 8786194 [details]
> Bug 1240412 - add VP9-in-MP4 support to Rust demuxer.
> 
> https://reviewboard.mozilla.org/r/74914/#review76228
> 
> The VPX decoder is no longer use for VP9/VP8 playback.
> 
> So this change will NOT allow to decode vp9 in mp4. You need to modify the
> FFVPXDecodeModule

I can't find FFVPXDecodeModule. I guess you mean FFVPXRuntimeLinker, which uses VPXDecoder::IsVP9 to check VP9 [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/1851b78b5a9673ee422f189b92e5f1e86b82a01c/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp#360
Flags: needinfo?(ayang)
I mean the FFVPXDecoderModule which is a FFMpegDecoderModule derivative that is created by the FFVPXRunTimeLinker::CreateDecodeModule()

But I was wrong, it uses VPXDecoder::IsVP9 so your change will be okay... 

just need to clean the mimetype to avoid using different mimetypes depending on the demuxer.
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #22)
> Alfredo, is this something manual QA could help test?

You don't need to do anything manually, vp9 in mp4 is supported by rust demuxer only, stagefright can't. So rust demuxer will be used in this case.
(In reply to Alfredo Yang (:alfredo) from comment #25)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #22)
> > Alfredo, is this something manual QA could help test?
> 
> You don't need to do anything manually, vp9 in mp4 is supported by rust
> demuxer only, stagefright can't. So rust demuxer will be used in this case.

Thank you for following up on this! Updating flags accordingly.
Flags: qe-verify? → qe-verify-
See Also: → 1302593
Depends on: 1304820
You need to log in before you can comment on or make changes to this bug.