| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
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
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
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 --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') },