1267887 - hook up rust mp4parse for opus

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

This bug is about landing the plumbing to let us use the rust mp4parse code when demuxing opus files.

- Prefer the opus demuxer when there is an opus track in mp4
- Return the necessary AudioInfo struct from GetTrackInfo()

Comment 1

[Ralph Giles (:rillian)]

Attachment: Part 1: Experiment with exporting track codec

Comment 2

[Ralph Giles (:rillian)]

Attachment: Part 2 - Hook up rust GetTrackInfo WIP

Comment 3

[Ralph Giles (:rillian)]

Anthony, I rebased by work from a few weeks ago. Gotta go for the evening, but if you want to take a look, you can start with this or pull the mp4-opus git branch from https://notabug.org/rillian/firefox

Comment 4

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

Ralph - do you have Opus/MP4 samples I can try?

Comment 5

[Ralph Giles (:rillian)]

I've been using https://people.mozilla.org/~rgiles/2016/sample.mp4 which Matthew made some time ago. He recently offered  https://flim.org/~kinetik/jaguar_opus.mp4 as well. It wasn't confirmed correct, though and while the rust demuxer finds an opus track some of the parameters are wonky.

Comment 6

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

I got 0 audio channels from https://flim.org/~kinetik/jaguar_opus.mp4

Comment 7

[Ralph Giles (:rillian)]

Added a simple bit to copy the metadata from the demux and it gets as far as starting to decode now. Next is to hook up the opus decoder.

Comment 8

[Ralph Giles (:rillian)]

Attachment: Part 2 - Hook up rust GetTrackInfo WIP v2

Comment 9

[Ralph Giles (:rillian)]

Attachment: Document MP4AudioInfo IsValid

Comment 10

[Ralph Giles (:rillian)]

Attachment: Sample opus-in-mp4 file for testing

Comment 11

[Ralph Giles (:rillian)]

Attachment: Part 2 - Hook up rust GetTrackInfo WIP v3

Rebase on top of bug 1274913.

Comment 12

[Ralph Giles (:rillian)]

The decoder initialization failure turned out to be because we weren't passing the codec_specific data with the Opus header data. We need to re-serialize this in RFC 7845 format, construct and prepend a codecDelay value (pre-skip converted to microseconds in a uint64_t) since the mp4 mapping doesn't store this redundantly like webm, and then pass that in the AudioInfo::mCodecSpecificConfig for the decoder to re-parse and validate. With this change the attached sample.mp4 sample file plays.

Thanks to k17e, kinetik, and jya for help tracking down the issue.

Comment 13

[Ralph Giles (:rillian)]

Attachment: Bug 1267887 - Experiment with exporting track mime-type.

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

Comment 14

[Ralph Giles (:rillian)]

Attachment: Bug 1267887 - Support Opus in mp4 with the rust demuxer.

Hook up the GetTrackInfo method to the rust demuxer results.

Fill in audio and video track metadata.

Prefer rust if it finds more tracks, and accept opus in mp4.

Pass audio codec_specific_config to the decoder.

With this change sample.mp4 plays.

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

Comment 15

[Ralph Giles (:rillian)]

Attachment: Bug 1267887 - Add serialization call to make an Opus header.

Backport serialization changes to the in-tree mp4parse.

Provide a convenience function to re-serialize the Opus
audio codec-specific config data for clients which need
to pass this on to a decoder.

Gecko wants this info in ogg/webm format, which means we need
to prepend the 'OpusHead' tag and endian-swap the multi-byte
fields relative to mp4.

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

Comment 16

[Ralph Giles (:rillian)]

Attachment: Bug 1267887 - Export Opus header through the capi.

Save the serialization of the opus header in a HashMap
under the track index as part of the context. This needs
to be per-track and outlive the call to mp4parse_get_track_audio_info()
so this seems the safest place. In mp4parse 0.3.x we can move
this to the capi's wrapper struct instead of cluttering up
the rust api.

Then copy this into an extra field in the audio_info struct
so it's available to the caller.

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

Comment 17

[Ralph Giles (:rillian)]

Attachment: Bug 1267887 - Document MP4AudioInfo::IsValid().

Jean-Yves and I both found this logic confusing.

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

Comment 18

[Ralph Giles (:rillian)]

Attachment: Bug 1267887 - Log short opus codec config block.

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

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8762894 [details]
Bug 1267887 - Document MP4AudioInfo::IsValid().

https://reviewboard.mozilla.org/r/59346/#review56342

Comment 20

[Matthew Gregan [:kinetik]]

Comment on attachment 8762890 [details]
Bug 1267887 - Experiment with exporting track mime-type.

https://reviewboard.mozilla.org/r/59338/#review56346

::: media/libstagefright/binding/mp4parse/capi.rs:187
(Diff revision 1)
>      };
>  
> +    info.codec = match &*context.tracks[track_index].mime_type {
> +        "audio/opus" => TRACK_CODEC_OPUS,
> +        "video/vp9" => TRACK_CODEC_VP9,
> +        "video/h264" => TRACK_CODEC_H264,

This is "video/avc" in read_video_desc.  Probably better to turn them in to constants so we can share the definition.

::: media/libstagefright/binding/mp4parse/capi.rs:188
(Diff revision 1)
>  
> +    info.codec = match &*context.tracks[track_index].mime_type {
> +        "audio/opus" => TRACK_CODEC_OPUS,
> +        "video/vp9" => TRACK_CODEC_VP9,
> +        "video/h264" => TRACK_CODEC_H264,
> +        "audio/aac" => TRACK_CODEC_AAC,

And this is audio/mp4a-latm

Comment 21

[Matthew Gregan [:kinetik]]

Comment on attachment 8762896 [details]
Bug 1267887 - Log short opus codec config block.

https://reviewboard.mozilla.org/r/59348/#review56354

Comment 22

[Matthew Gregan [:kinetik]]

Comment on attachment 8762893 [details]
Bug 1267887 - Export Opus header through the capi.

https://reviewboard.mozilla.org/r/59344/#review56356

::: media/libstagefright/binding/include/mp4parse.h:34
(Diff revision 1)
>  #define MP4PARSE_TRACK_CODEC_AAC 1
>  #define MP4PARSE_TRACK_CODEC_OPUS 2
>  #define MP4PARSE_TRACK_CODEC_H264 3
>  #define MP4PARSE_TRACK_CODEC_VP9 4
>  
> +struct CodecSpecific {

mp4parse_codec_specific_config or something

::: media/libstagefright/binding/mp4parse/capi.rs:228
(Diff revision 1)
>  
>      MP4PARSE_OK
>  }
>  
>  #[no_mangle]
> -pub unsafe extern "C" fn mp4parse_get_track_audio_info(context: *mut MediaContext, track: u32, info: *mut TrackAudioInfo) -> i32 {
> +pub unsafe extern "C" fn mp4parse_get_track_audio_info(context: *mut MediaContext, track_index: u32, info: *mut TrackAudioInfo) -> i32 {

I guess change track to track_index everywhere to keep things consistent.

::: media/libstagefright/binding/mp4parse/capi.rs:275
(Diff revision 1)
> -            println!("{:?}", opus);
> -            (*info).codec_specific_config.length = 0;
> -            (*info).codec_specific_config.data = std::ptr::null();
> +            match serialize_opus_header(opus, &mut v) {
> +                Err(_) => {
> +                    return MP4PARSE_ERROR_INVALID;
> +                }
> +                Ok(_) => {
> +            context.opus_header.insert(track_index, v);

Indentation looks off here

Comment 23

[Matthew Gregan [:kinetik]]

Comment on attachment 8762892 [details]
Bug 1267887 - Add serialization call to make an Opus header.

https://reviewboard.mozilla.org/r/59342/#review56380

::: media/libstagefright/binding/mp4parse/lib.rs:1045
(Diff revision 1)
> +///
> +/// Some decoders expect the initialization data in the format used by the
> +/// Ogg and WebM encapsulations. To support this we prepend the 'OpusHead'
> +/// tag and byte-swap the data from big- to little-endian relative to the
> +/// dOps box.
> +fn serialize_opus_header<W: byteorder::WriteBytesExt + std::io::Write>(opus: &OpusSpecificBox, dst: &mut W) -> Result<()> {

Since this isn't related to MP4 at all, it might be better to put it directly in capi.rs.

::: media/libstagefright/binding/mp4parse/lib.rs:1054
(Diff revision 1)
> +            if bytes != 8 {
> +                return Err(Error::InvalidData);
> +            }
> +        }
> +    }
> +    try!(dst.write_u8(opus.version));

I think you should write version == 1 here, as this value is the version of the OpusHead structure.  The value in opus.version is the version of dOps, a similar but unrelated structure with parallel versioning.

Comment 24

[Matthew Gregan [:kinetik]]

Comment on attachment 8762891 [details]
Bug 1267887 - Support Opus in mp4 with the rust demuxer.

https://reviewboard.mozilla.org/r/59340/#review56350

::: media/libstagefright/binding/MP4Metadata.cpp:197
(Diff revision 1)
>      }
>  
>      mReportedTelemetry = true;
>    }
> +
> +  if (numTracksRust > numTracks) {

I'd prefer it if we detected VPx/Opus more explicitly and preferred mp4parse-rust in that case only.

::: media/libstagefright/binding/MP4Metadata.cpp:544
(Diff revision 1)
>    uint32_t tracks = mp4parse_get_track_count(mRustState.get());
>    MOZ_LOG(sLog, LogLevel::Info, ("rust parser found %u tracks", tracks));
>  
>    uint32_t total = 0;
>    for (uint32_t i = 0; i < tracks; ++i) {
> -    mp4parse_track_info track_info;
> +    struct mp4parse_track_info track_info;

None of the mp4parse_ structs should need to be prefixed with structs, since they're typedefs.

::: media/libstagefright/binding/mp4parse/capi.rs:193
(Diff revision 1)
>          TrackType::Unknown => return MP4PARSE_ERROR_UNSUPPORTED,
>      };
>  
>      info.codec = match &*context.tracks[track_index].mime_type {
>          "audio/opus" => TRACK_CODEC_OPUS,
> +        "audio/aac" |

We're not producing audio/aac or video/h264 as MIME strings anywhere in the parser, so I don't understand why we'd accept them here.

Comment 25

[Ralph Giles (:rillian)]

Attachment: Experiment with exporting track codec

Carrying r=kinetik from bug 1275812.

Comment 26

[Ralph Giles (:rillian)]

Attachment: Support Opus in mp4 v2

Rebase on top of mp4parse v0.4.0, incorporating review comments.

Comment 27

[Ralph Giles (:rillian)]

Attachment: Part 1 - Update script for mp4parse v0.4.0

Comment 28

[Ralph Giles (:rillian)]

Attachment: Result of running update script

Comment 29

[Matthew Gregan [:kinetik]]

Comment on attachment 8765644 [details] [diff] [review]
Support Opus in mp4 v2

Review of attachment 8765644 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/DecoderData.cpp
@@ +187,5 @@
>  }
>  
> +#ifdef MOZ_RUST_MP4PARSE
> +void
> +MP4VideoInfo::Update(const struct mp4parse_track_info* track,

These don't need struct on them, even in pure C, because they're typedefs.

@@ +188,5 @@
>  
> +#ifdef MOZ_RUST_MP4PARSE
> +void
> +MP4VideoInfo::Update(const struct mp4parse_track_info* track,
> +                     const struct mp4parse_track_video_info* video)

Ditto.

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +117,5 @@
> +
> +  bool Read(uint8_t* buffer, uintptr_t size, size_t* bytes_read);
> +
> +private:
> +  Stream *mSource;

Stream* mSource;

@@ +142,5 @@
>  private:
>    CryptoFile mCrypto;
>    RefPtr<Stream> mSource;
> +  RustStreamAdaptor mRustSource;
> +  const mp4parse_io mRustIo;

mp4parse_new copies the mp4parse_io contents, so there's no need to keep one around on this side and the code is clearer if we don't.

@@ +217,5 @@
>      mReportedTelemetry = true;
>    }
> +
> +  if (mPreferRust || ShouldPreferRust()) {
> +    MOZ_LOG(sLog, LogLevel::Info, ("Prefering rust demuxer"));

Preferring

@@ +443,4 @@
>    if (e && e->IsValid()) {
>      return e;
>    }
> +

Whitespace?

@@ +597,5 @@
>    MOZ_LOG(sLog, LogLevel::Info, ("rust parser found %u tracks", tracks));
>  
>    uint32_t total = 0;
>    for (uint32_t i = 0; i < tracks; ++i) {
> +    struct mp4parse_track_info track_info;

Doesn't need struct, it's a typedef.

@@ +627,5 @@
>                                size_t aTrackNumber) const
>  {
> +  static LazyLogModule sLog("MP4Metadata");
> +
> +  struct mp4parse_track_info info;

No struct

@@ +650,5 @@
> +  // This specialization interface is crazy.
> +  UniquePtr<mozilla::TrackInfo> e;
> +  switch (aType) {
> +    case TrackInfo::TrackType::kAudioTrack: {
> +      struct mp4parse_track_audio_info audio;

No struct

@@ +663,5 @@
> +        // The Opus decoder expects the container's codec delay or
> +        // pre-skip value, in microseconds, as a 64-bit int at the
> +        // start of the codec-specific config blob.
> +        MOZ_ASSERT(audio.codec_specific_config.data);
> +        MOZ_ASSERT(audio.codec_specific_config.length >= 12);

The Rust code included in *this* patch is setting this to nullptr/0, so doesn't this assert always trigger?

@@ +671,5 @@
> +            ("Copying opus pre-skip value of %d as CodecDelay.",(int)preskip));
> +        uint8_t codecDelay[sizeof(uint64_t)];
> +        BigEndian::writeUint64(codecDelay,
> +            mozilla::FramesToUsecs(preskip, 48000).value());
> +        track->mCodecSpecificConfig->AppendElements(codecDelay, sizeof(uint64_t));

This chunk should be abstracted out so we can share it with the WebM demuxer.

@@ +687,5 @@
> +      e = Move(track);
> +    }
> +    break;
> +    case TrackInfo::TrackType::kVideoTrack: {
> +      struct mp4parse_track_video_info video;

No struct

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
@@ +22,5 @@
> +#ifdef MOZ_RUST_MP4PARSE
> +extern "C" {
> +struct mp4parse_track_info;
> +struct mp4parse_track_audio_info;
> +struct mp4parse_track_video_info;

Technically these should be: typedef struct foo foo; to match the mp4parse.h header.

@@ +81,5 @@
>                const char* aMimeType);
>  
> +#ifdef MOZ_RUST_MP4PARSE
> +  void Update(const struct mp4parse_track_info* track,
> +              const struct mp4parse_track_video_info* video);

Should need struct on these either.

::: media/libstagefright/binding/mp4parse/capi.rs
@@ +195,3 @@
>          "video/vp9" => TRACK_CODEC_VP9,
> +        "video/h264" |
> +        "video/avc" => TRACK_CODEC_H264,

Remove video/h264 and audio/aac, because the parser never produces these strings so it doesn't make sense to match on them.

::: media/libstagefright/gtest/TestMP4Rust.cpp
@@ +113,4 @@
>    mp4parse_free(context);
>  }
>  
> +

Whitespace?

It's a bit of a shame we have to duplicate this testing in multiple places; this looks quite similar to what we're testing in the upstream repo already.

Comment 30

[Matthew Gregan [:kinetik]]

Comment on attachment 8765649 [details] [diff] [review]
Result of running update script

Review of attachment 8765649 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/opus-header.patch
@@ +1,1 @@
> +diff --git a/media/libstagefright/binding/mp4parse/lib.rs b/media/libstagefright/binding/mp4parse/lib.rs

Both of the opus-header patches are obsolete and should be removed, right? The code is upstream now.

Comment 31

[Ralph Giles (:rillian)]

(In reply to Matthew Gregan [:kinetik] from comment #30)

> Both of the opus-header patches are obsolete and should be removed, right?
> The code is upstream now.

That's right. Looks like a rebase oversight.

Comment 32

[Ralph Giles (:rillian)]

Attachment: Support Opus in mp4 v3

Update patch addressing review comments. Carrying r=kinetik.

(In reply to Matthew Gregan [:kinetik] from comment #29)
> Comment on attachment 8765644 [details] [diff] [review]
> Support Opus in mp4 v2
> [...]
> > +MP4VideoInfo::Update(const struct mp4parse_track_info* track,
> 
> These don't need struct on them, even in pure C, because they're typedefs.

Ok, removed.
 
> mp4parse_new copies the mp4parse_io contents, so there's no need to keep one
> around on this side and the code is clearer if we don't.

I did think because I couldn't figure out a nice way to initialize the UniquePtr in the ctor body. Replaced with `mRustParser.reset(mp4parse_new(...));` after irc discussion.
 
> @@ +663,5 @@
> > +        // The Opus decoder expects the container's codec delay or
> > +        // pre-skip value, in microseconds, as a 64-bit int at the
> > +        // start of the codec-specific config blob.
> > +        MOZ_ASSERT(audio.codec_specific_config.data);
> > +        MOZ_ASSERT(audio.codec_specific_config.length >= 12);
> 
> The Rust code included in *this* patch is setting this to nullptr/0, so
> doesn't this assert always trigger?

The rust code included in the previous version of the patch was a rebase error. Should be better now.

> 
> @@ +671,5 @@
> > +            ("Copying opus pre-skip value of %d as CodecDelay.",(int)preskip));
> > +        uint8_t codecDelay[sizeof(uint64_t)];
> > +        BigEndian::writeUint64(codecDelay,
> > +            mozilla::FramesToUsecs(preskip, 48000).value());
> > +        track->mCodecSpecificConfig->AppendElements(codecDelay, sizeof(uint64_t));
> 
> This chunk should be abstracted out so we can share it with the WebM demuxer.

I'd like to do that in a follow up bug.

> ::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
> @@ +22,5 @@
> > +#ifdef MOZ_RUST_MP4PARSE
> > +extern "C" {
> > +struct mp4parse_track_info;
> > +struct mp4parse_track_audio_info;
> > +struct mp4parse_track_video_info;
> 
> Technically these should be: typedef struct foo foo; to match the mp4parse.h
> header.

Ok, updated. They seem to work without even after removing all the 'struct' keywords above. Perhaps that's just because of the unified build.

> ::: media/libstagefright/binding/mp4parse/capi.rs
> @@ +195,3 @@
> >          "video/vp9" => TRACK_CODEC_VP9,
> > +        "video/h264" |
> > +        "video/avc" => TRACK_CODEC_H264,
> 
> Remove video/h264 and audio/aac, because the parser never produces these
> strings so it doesn't make sense to match on them.

Spurious. I've removed the whole hunk. Sorry about that.

> ::: media/libstagefright/gtest/TestMP4Rust.cpp
> [...]
> 
> It's a bit of a shame we have to duplicate this testing in multiple places;
> this looks quite similar to what we're testing in the upstream repo already.

I agree, but I think it's important to have something in-tree as long as we have separate change-control for the two repos. Maybe once we get cargo integration we can have `cargo test` in the gecko tier-1 jobs and then we can drop the gtest.

Comment 33

[Ralph Giles (:rillian)]

Attachment: Update script for mp4parse v0.4.0 v3

Clean up issues with update script patch. No functional changes. Carrying forward r=kinetik.

Comment 34

[Ralph Giles (:rillian)]

Attachment: Result of running update script v3

Rerun import script to get a clean patch after the other changes. Carrying forward r=kinetik.

Comment 35

[Ralph Giles (:rillian)]

Attachment: Result of running update script v3bis

Forgot to add boxes.rs when redoing the patch.

Comment 36

[Ralph Giles (:rillian)]

Attachment: Add algorithm.h for std::min on msvc

One last fix from try: MSVC apparently needs an include for std::min in TestMP4Rust.cpp.

With this change it looks ready to land:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cda3e0f1478 (windows build broken without this patch)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5153e9163f34&selectedJob=23074431 (windows build fixed with this patch)

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e19c4c4eac51
Experiment with exporting track mime-type. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/72ea61f4f820
Support Opus in mp4 with the rust demuxer. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1177fd30cb
Update script for mp4parse v0.4.0. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/c697af8c555a
Document MP4AudioInfo::IsValid(). r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/9128f4f94032
Log short opus codec config blocks. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e34b315a0a
Add algorithm.h for std::min on msvc. r=kinetik

Comment 39

[Phil Ringnalda (:philor)]

Yeouch. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/348930b0d9db48ced9ec44b5b499517b8e828906 for crashing (without, of course, a stack) on every Win8 suite which involves Marionette behind the scenes, with a useless stack in the actual Marionette suite, and failing or crashing in gtests.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74b1094b2230
Experiment with exporting track mime-type. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/66779241e5ae
Support Opus in mp4 with the rust demuxer. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/f44b0aa911fa
Update script for mp4parse v0.4.0. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7beb30ea06
Update source for mp4parse v0.4.0. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9dde9bc5962
Document MP4AudioInfo::IsValid(). r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/907357db4cb8
Log short opus codec config blocks. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c153c84b7d
Add algorithm.h for std::min on msvc. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca86cc8dde7
Bump CLOBBER. r=me

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74b1094b2230
https://hg.mozilla.org/mozilla-central/rev/66779241e5ae
https://hg.mozilla.org/mozilla-central/rev/f44b0aa911fa
https://hg.mozilla.org/mozilla-central/rev/7e7beb30ea06
https://hg.mozilla.org/mozilla-central/rev/b9dde9bc5962
https://hg.mozilla.org/mozilla-central/rev/907357db4cb8
https://hg.mozilla.org/mozilla-central/rev/74c153c84b7d
https://hg.mozilla.org/mozilla-central/rev/6ca86cc8dde7