diff mbox series

[FFmpeg-devel] avformat/movenc: allow Apple Lossless inside mp4

Message ID 20201120163831.91560-1-leo.izen@gmail.com
State Accepted
Commit f5dcaf2daa34d9cbe6e6ad13ea2c8a7ee43d0064
Headers show
Series [FFmpeg-devel] avformat/movenc: allow Apple Lossless inside mp4
Related show

Checks

Context Check Description
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Leo Izen Nov. 20, 2020, 4:38 p.m. UTC
MP4 already supports Apple Lossless, and it can be put inside by using
muxer=ipod, but it's not tagged as supported in mp4. The mp4ra lists
alac as a valid fourcc inside mp4, so it should be supported by spec.

See: https://mp4ra.org/#/codecs
---
 libavformat/movenc.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jan Ekström Nov. 20, 2020, 8:53 p.m. UTC | #1
On Fri, Nov 20, 2020 at 7:02 PM Leo Izen <leo.izen@gmail.com> wrote:
>
> MP4 already supports Apple Lossless, and it can be put inside by using
> muxer=ipod, but it's not tagged as supported in mp4. The mp4ra lists
> alac as a valid fourcc inside mp4, so it should be supported by spec.
>
> See: https://mp4ra.org/#/codecs

I would just note in the paragraph that the identifier has been
registered to ISO and thus towards ISOBMFF (which we usually call
"mp4") at the MP4 registration authority.

This can be changed when applying the patch, so no second revision
needed for this change.

> ---
>  libavformat/movenc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index fea8a86192..18fa3f9b5e 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -7128,6 +7128,7 @@ static const AVCodecTag codec_mp4_tags[] = {
>      { AV_CODEC_ID_VP9,             MKTAG('v', 'p', '0', '9') },
>      { AV_CODEC_ID_AV1,             MKTAG('a', 'v', '0', '1') },
>      { AV_CODEC_ID_AAC,             MKTAG('m', 'p', '4', 'a') },
> +    { AV_CODEC_ID_ALAC,            MKTAG('a', 'l', 'a', 'c') },
>      { AV_CODEC_ID_MP4ALS,          MKTAG('m', 'p', '4', 'a') },
>      { AV_CODEC_ID_MP3,             MKTAG('m', 'p', '4', 'a') },
>      { AV_CODEC_ID_MP2,             MKTAG('m', 'p', '4', 'a') },
> --
> 2.29.2
>

Not sure of the location in the list (this matches the ipod muxer's
ordering, while additional formats seem to have just been added into
their list/group as they popped up - that said since this is a static
symbol I would expect even if the addition was in the middle, it
should be a-OK), but after taking a quick look at the ALAC code paths
this seems to cause writing of boxes that seem to make sense (audio
description plus whatever is in the extradata).

I will attempt to check tomorrow if there's a spec for this to
double-check against, but preliminarily this looks good to me.

Jan
Jan Ekström Nov. 21, 2020, 7:08 p.m. UTC | #2
On Fri, Nov 20, 2020 at 10:53 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 7:02 PM Leo Izen <leo.izen@gmail.com> wrote:
> >
> > MP4 already supports Apple Lossless, and it can be put inside by using
> > muxer=ipod, but it's not tagged as supported in mp4. The mp4ra lists
> > alac as a valid fourcc inside mp4, so it should be supported by spec.
> >
> > See: https://mp4ra.org/#/codecs
>
> I would just note in the paragraph that the identifier has been
> registered to ISO and thus towards ISOBMFF (which we usually call
> "mp4") at the MP4 registration authority.
>
> This can be changed when applying the patch, so no second revision
> needed for this change.
>
> > ---
> >  libavformat/movenc.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index fea8a86192..18fa3f9b5e 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -7128,6 +7128,7 @@ static const AVCodecTag codec_mp4_tags[] = {
> >      { AV_CODEC_ID_VP9,             MKTAG('v', 'p', '0', '9') },
> >      { AV_CODEC_ID_AV1,             MKTAG('a', 'v', '0', '1') },
> >      { AV_CODEC_ID_AAC,             MKTAG('m', 'p', '4', 'a') },
> > +    { AV_CODEC_ID_ALAC,            MKTAG('a', 'l', 'a', 'c') },
> >      { AV_CODEC_ID_MP4ALS,          MKTAG('m', 'p', '4', 'a') },
> >      { AV_CODEC_ID_MP3,             MKTAG('m', 'p', '4', 'a') },
> >      { AV_CODEC_ID_MP2,             MKTAG('m', 'p', '4', 'a') },
> > --
> > 2.29.2
> >
>
> Not sure of the location in the list (this matches the ipod muxer's
> ordering, while additional formats seem to have just been added into
> their list/group as they popped up - that said since this is a static
> symbol I would expect even if the addition was in the middle, it
> should be a-OK), but after taking a quick look at the ALAC code paths
> this seems to cause writing of boxes that seem to make sense (audio
> description plus whatever is in the extradata).
>
> I will attempt to check tomorrow if there's a spec for this to
> double-check against, but preliminarily this looks good to me.

Verified against the spec
(https://github.com/macosforge/alac/blob/master/ALACMagicCookieDescription.txt#L177).

As we already had the non-legacy (mode != MOV) support in place, the
output - as expected - matches the specification (checked with
L-SMASH's boxdumper).

Thus, the change itself is LGTM.

Jan
Jan Ekström Nov. 22, 2020, 12:33 p.m. UTC | #3
On Sat, Nov 21, 2020 at 9:08 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 10:53 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Fri, Nov 20, 2020 at 7:02 PM Leo Izen <leo.izen@gmail.com> wrote:
> > >
> > > MP4 already supports Apple Lossless, and it can be put inside by using
> > > muxer=ipod, but it's not tagged as supported in mp4. The mp4ra lists
> > > alac as a valid fourcc inside mp4, so it should be supported by spec.
> > >
> > > See: https://mp4ra.org/#/codecs
> >
> > I would just note in the paragraph that the identifier has been
> > registered to ISO and thus towards ISOBMFF (which we usually call
> > "mp4") at the MP4 registration authority.
> >
> > This can be changed when applying the patch, so no second revision
> > needed for this change.
> >
> > > ---
> > >  libavformat/movenc.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > > index fea8a86192..18fa3f9b5e 100644
> > > --- a/libavformat/movenc.c
> > > +++ b/libavformat/movenc.c
> > > @@ -7128,6 +7128,7 @@ static const AVCodecTag codec_mp4_tags[] = {
> > >      { AV_CODEC_ID_VP9,             MKTAG('v', 'p', '0', '9') },
> > >      { AV_CODEC_ID_AV1,             MKTAG('a', 'v', '0', '1') },
> > >      { AV_CODEC_ID_AAC,             MKTAG('m', 'p', '4', 'a') },
> > > +    { AV_CODEC_ID_ALAC,            MKTAG('a', 'l', 'a', 'c') },
> > >      { AV_CODEC_ID_MP4ALS,          MKTAG('m', 'p', '4', 'a') },
> > >      { AV_CODEC_ID_MP3,             MKTAG('m', 'p', '4', 'a') },
> > >      { AV_CODEC_ID_MP2,             MKTAG('m', 'p', '4', 'a') },
> > > --
> > > 2.29.2
> > >
> >
> > Not sure of the location in the list (this matches the ipod muxer's
> > ordering, while additional formats seem to have just been added into
> > their list/group as they popped up - that said since this is a static
> > symbol I would expect even if the addition was in the middle, it
> > should be a-OK), but after taking a quick look at the ALAC code paths
> > this seems to cause writing of boxes that seem to make sense (audio
> > description plus whatever is in the extradata).
> >
> > I will attempt to check tomorrow if there's a spec for this to
> > double-check against, but preliminarily this looks good to me.
>
> Verified against the spec
> (https://github.com/macosforge/alac/blob/master/ALACMagicCookieDescription.txt#L177).
>
> As we already had the non-legacy (mode != MOV) support in place, the
> output - as expected - matches the specification (checked with
> L-SMASH's boxdumper).
>
> Thus, the change itself is LGTM.

Applied as f5dcaf2daa34d9cbe6e6ad13ea2c8a7ee43d0064 .

Jan
diff mbox series

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index fea8a86192..18fa3f9b5e 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -7128,6 +7128,7 @@  static const AVCodecTag codec_mp4_tags[] = {
     { AV_CODEC_ID_VP9,             MKTAG('v', 'p', '0', '9') },
     { AV_CODEC_ID_AV1,             MKTAG('a', 'v', '0', '1') },
     { AV_CODEC_ID_AAC,             MKTAG('m', 'p', '4', 'a') },
+    { AV_CODEC_ID_ALAC,            MKTAG('a', 'l', 'a', 'c') },
     { AV_CODEC_ID_MP4ALS,          MKTAG('m', 'p', '4', 'a') },
     { AV_CODEC_ID_MP3,             MKTAG('m', 'p', '4', 'a') },
     { AV_CODEC_ID_MP2,             MKTAG('m', 'p', '4', 'a') },