1303888 - implement flac-in-mp4 spec in the rust mp4 parser

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

Get started on an implementation so we can test this and evaluate the design.

Comment 1

[Ralph Giles (:rillian)]

WIP at https://github.com/rillian/mp4parse-rust/tree/flac

Will work through PRs on the make mp4parse repo and close this bug with an update of the gecko in-tree version.

Comment 2

[Vittorio Giovara]

Is there any plan for a patch in ffmpeg/libav?

Comment 3

[Ralph Giles (:rillian)]

(In reply to Vittorio Giovara from comment #2)
> Is there any plan for a patch in ffmpeg/libav?

That would be a separate bug. I'm not aware of anyone at Mozilla working on it; hopefully someone else can take that on.

Comment 4

[Ralph Giles (:rillian)]

Preliminary support landed in the upstream repo with https://github.com/mozilla/mp4parse-rust/pull/32 Working on playback integration in Firefox is next.

Comment 5

[Ralph Giles (:rillian)]

Attachment: Bug 1303888 - Accept flac from the rust mp4parse demuxer.

Recognize FLAC as an audio mimetype in MP4Metadata and prefer
the rust demuxer for it. Stagefright does not support this.

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

Comment 6

[Ralph Giles (:rillian)]

Attachment: Bug 1303888 - Fix a logging typo.

The error message refers to video, not audio tracks. Looks like
a cut-and-paste error.

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

Comment 7

[Ralph Giles (:rillian)]

With these changes I can play a test file generated by kinetik's ffmpeg branch.

This also depends on the mp4parse changes in https://github.com/mozilla/mp4parse-rust/pull/42 I'll add a patch to update the gecko copy once that's merged upstream.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8804505 [details]
Bug 1303888 - Fix a logging typo.

https://reviewboard.mozilla.org/r/88452/#review87494

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8804504 [details]
Bug 1303888 - Accept flac from the rust mp4parse demuxer.

https://reviewboard.mozilla.org/r/88450/#review87496

::: media/libstagefright/binding/MP4Metadata.cpp:251
(Diff revision 1)
>        return false;
>      }
>      if (info->mMimeType.EqualsASCII("audio/opus")) {
>        return true;
>      }
> +    if (info->mMimeType.EqualsASCII("audio/flac")) {

This could be:

    if (info->mMimeType.EqualsASCII("audio/opus") ||
        info->mMimeType.EqualsASCII("audio/flac")) {
      return true;
    }

Comment 10

[Ralph Giles (:rillian)]

Ok, thanks.

Comment 11

[Ralph Giles (:rillian)]

Attachment: Bug 1303888 - Update rust mp4parse.

Update to the latest upstream master, including skipping padding
at the end of some boxes, esds parsing and flac support.

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

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8804504 [details]
Bug 1303888 - Accept flac from the rust mp4parse demuxer.

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

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8804505 [details]
Bug 1303888 - Fix a logging typo.

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

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8804530 [details]
Bug 1303888 - Update rust mp4parse.

https://reviewboard.mozilla.org/r/88464/#review87534

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2330e758e138
Update rust mp4parse. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/d047232476ba
Accept flac from the rust mp4parse demuxer. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/718bf391399a
Fix a logging typo. r=kinetik

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2330e758e138
https://hg.mozilla.org/mozilla-central/rev/d047232476ba
https://hg.mozilla.org/mozilla-central/rev/718bf391399a

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8804504 [details]
Bug 1303888 - Accept flac from the rust mp4parse demuxer.

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: FLAC in MP4 and its use with MSE not available
[Describe test coverage new/current, TreeHerder]: In central for a while. Confirmed with 3rd party as working.
[Risks and why]: Low. This is a new feature. We've been working with partners to expose FLAC with MSE
[String/UUID change made/needed]: none

Comment 18

[Gerry Chang [:gchang]]

Hi :jya,
In general, we only accept uplift request for existing features or regressions in aurora/beta. From my understanding, is bug 1195723 the feature of this bug?

Comment 19

[Jean-Yves Avenard [:jya]]

We have made commitment with some partners to enable flac in Firefox. Flac was first added in bug 1195723, however it isn't available with MSE.

We have made a lot of work to get flac ready, unfortunately the last blob took longer than expected as it required lots of parties to coordinate their effort.  As such this bug is a continuation of bug 1195723

Now that the feature is available in nightly, and has been for a while, and also confirmed to work by our partners. We now would like to fast track this availability, otherwise the service won't be able to be provided by our partner.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8804504 [details]
Bug 1303888 - Accept flac from the rust mp4parse demuxer.

Support flac in mp4. Since the feature has been in nightly for a while and is confirmed to work by our partners we can take it in 51 aurora.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora. Or bug 1306164 needs uplift too.

Comment 22

[Ralph Giles (:rillian)]

We should uplift bugs 1306164, 1306755, 1303888, 1314460, and 1315567 to reduce variance between the 51 and 52 implementations.

Comment 23

[Ralph Giles (:rillian)]

I have a set of backport patches to enable this feature in Aurora 51. Carrying forward a=gchang for the necessary dependencies.

Comment 24

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/39384a565895fc84a42bc52febc59d11ab5b8fe5
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1a84f7fff9505dff8ac4315b9a7be611ab7d8ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d39e45e752d48c6f649e98cae0ea75ffbf2daf2
https://hg.mozilla.org/releases/mozilla-aurora/rev/9a64cd121744362855402a14c699769dcbad8771
https://hg.mozilla.org/releases/mozilla-aurora/rev/de0b86f887a0e67b13db75fe3032d2f4096d711d
https://hg.mozilla.org/releases/mozilla-aurora/rev/a632178a4f48bc53360871435e97ea65abd05a71
https://hg.mozilla.org/releases/mozilla-aurora/rev/11f06ea89d23b457eef4759e6ea8eee2dc7b1515
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ec059299a275c49adaee9c737cc94b7ce551231
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c1ca21cf9c30f6056e9b4cef7280829a2b60ca7

Comment 25

[Carsten Book [:Tomcat]]

also setting the flag for ralph

(In reply to Ralph Giles (:rillian) needinfo me from comment #24)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 39384a565895fc84a42bc52febc59d11ab5b8fe5
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> e1a84f7fff9505dff8ac4315b9a7be611ab7d8ed
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 3d39e45e752d48c6f649e98cae0ea75ffbf2daf2
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 9a64cd121744362855402a14c699769dcbad8771
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> de0b86f887a0e67b13db75fe3032d2f4096d711d
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> a632178a4f48bc53360871435e97ea65abd05a71
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 11f06ea89d23b457eef4759e6ea8eee2dc7b1515
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 3ec059299a275c49adaee9c737cc94b7ce551231
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 9c1ca21cf9c30f6056e9b4cef7280829a2b60ca7

Comment 26

[Eric Shepherd [:sheppy]]

I’ve added this to the article about supported media formats:

https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats#MP4_FLAC

And in the table at:

https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats#Browser_compatibility (see “<video>: FLAC in MP4” row near the bottom)

And this is now listed on Firefox 51 for developers.

Please advise if there are details missing or errors made.

Comment 27

[Jean-Yves Avenard [:jya]]

Thank you for this.

Flac in MP4 can also be listed as supported in the audio element. Right below flac in off (or if we want to group them by container, below aac in MP4)