Closed Bug 1303888 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Get started on an implementation so we can test this and evaluate the design.
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.
Assignee: nobody → giles
Is there any plan for a patch in ffmpeg/libav?
(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.
Preliminary support landed in the upstream repo with https://github.com/mozilla/mp4parse-rust/pull/32 Working on playback integration in Firefox is next.
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 on attachment 8804505 [details]
Bug 1303888 - Fix a logging typo.

https://reviewboard.mozilla.org/r/88452/#review87494
Attachment #8804505 - Flags: review?(kinetik) → review+
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;
    }
Attachment #8804504 - Flags: review?(kinetik) → review+
Ok, thanks.
Comment on attachment 8804530 [details]
Bug 1303888 - Update rust mp4parse.

https://reviewboard.mozilla.org/r/88464/#review87534
Attachment #8804530 - Flags: review?(kinetik) → review+
Pushed by rgiles@mozilla.com:
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 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
Attachment #8804504 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
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.
Attachment #8804504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora. Or bug 1306164 needs uplift too.
Flags: needinfo?(giles)
We should uplift bugs 1306164, 1306755, 1303888, 1314460, and 1315567 to reduce variance between the 51 and 52 implementations.
Flags: needinfo?(giles)
I have a set of backport patches to enable this feature in Aurora 51. Carrying forward a=gchang for the necessary dependencies.
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
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.
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)
You need to log in before you can comment on or make changes to this bug.