1240412 - (vp9-in-mp4) Add VP9-in-MP4 support to Rust demuxer

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Comment 1

[Ralph Giles (:rillian)]

I've got some WIP patches in https://notabug.org/rillian/mp4parse-rust/src/vp9

Comment 2

[Chris Peterson [:cpeterson]]

P2 because VP9-in-MP4 is important, but not a release blocker.

Comment 3

[Ralph Giles (:rillian)]

We have mp4parse-rust reading vp9 Sample Entries as of 0.2.0.

Comment 4

[Chris Peterson [:cpeterson]]

Chromium landed support for VP9 in MP4 yesterday (pref'd off):

https://bugs.chromium.org/p/chromium/issues/detail?id=580623

Comment 5

[Ralph Giles (:rillian)]

Wow, that's great!

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 7

[Ralph Giles (:rillian)]

Matthew, are you still able to work on this?

Comment 8

[Matthew Gregan [:kinetik]]

Sure.

Comment 9

[Alfredo Yang (:alfredo)]

Reassign to the correct person. :-)

Comment 10

[Ralph Giles (:rillian)]

Sorry, Alfredo and Alastor! I was too quick with the auto-complete.

Comment 11

[Alfredo Yang (:alfredo)]

Attachment: Bug 1240412 - add VP9-in-MP4 support to Rust demuxer.

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

Comment 12

[Alfredo Yang (:alfredo)]

Attachment: bear-320x240-v_frag-vp9.mp4

The test video is from test sample in chromium.

Comment 13

[Alfredo Yang (:alfredo)]

ffvpx fails to decode the vp9 sample in https://github.com/Netflix/vp9-dash/tree/master/DASH-Samples.
libvpx is ok.

Comment 14

[Matthew Gregan [:kinetik]]

(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 15

[Ralph Giles (:rillian)]

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.

Comment 16

[Vittorio Giovara]

As Vimeo is looking into VP9, I really hope this feature sees the light (and enabled by default unlike in Chrome).

Comment 17

[Alfredo Yang (:alfredo)]

Comment on attachment 8786194 [details]
Bug 1240412 - add VP9-in-MP4 support to Rust demuxer.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c82d3d5d0886
add VP9-in-MP4 support to Rust demuxer. r=rillian

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c82d3d5d0886

Comment 20

[Jean-Yves Avenard [:jya]]

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 21

[Jean-Yves Avenard [:jya]]

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

Comment 22

[Andrei Vaida [:avaida]]

Alfredo, is this something manual QA could help test?

Comment 23

[Alfredo Yang (:alfredo)]

(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

Comment 24

[Jean-Yves Avenard [:jya]]

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.

Comment 25

[Alfredo Yang (:alfredo)]

(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.

Comment 26

[Andrei Vaida [:avaida]]

(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.