| Message ID | 20230207205934.7584-1-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Commit | 02ddfeadbee52c1ad8c023a93594a8cb957e11db |
| Headers | show |
| Series | [FFmpeg-devel] avformat/movenc: allow writing out channel count in MP4 and 3GP | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
On Tue, 7 Feb 2023, Jan Ekström wrote: > ISOBMFF (14496-12) made this field ('channelcount') in the > AudioSampleEntry structure non-template¹ somewhere before the > release of the 2022 edition. As for ETSI TS 126 244 AKA 3GPP > file format (V16.1.0, 2020-10), it does not seem contain any > references limiting the channelcount entry in AudioSampleEntry > or in its own definition of EVSSampleEntry. > > fate-mov-mp4-chapters test had to be adjusted as it output a > mono vorbis stream, which would now be properly marked as such > in the container. > > 1: As per 14496-12: > Fields shown as “template” in the box descriptions are fields > which are coded with a default value unless a derived > specification defines their use and permits writers to use > other values than the default. > --- > libavformat/movenc.c | 8 +------- > tests/ref/fate/mov-mp4-chapters | 2 +- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c > index 8d31317838..f0e218e7b7 100644 > --- a/libavformat/movenc.c > +++ b/libavformat/movenc.c > @@ -1241,13 +1241,7 @@ static int mov_write_audio_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContex > avio_wb16(pb, 16); > avio_wb16(pb, track->audio_vbr ? -2 : 0); /* compression ID */ > } else { /* reserved for mp4/3gp */ > - if (track->par->codec_id == AV_CODEC_ID_FLAC || > - track->par->codec_id == AV_CODEC_ID_ALAC || > - track->par->codec_id == AV_CODEC_ID_OPUS) { > - avio_wb16(pb, track->par->ch_layout.nb_channels); > - } else { > - avio_wb16(pb, 2); > - } > + avio_wb16(pb, track->par->ch_layout.nb_channels); > if (track->par->codec_id == AV_CODEC_ID_FLAC || > track->par->codec_id == AV_CODEC_ID_ALAC) { > avio_wb16(pb, track->par->bits_per_raw_sample); This looks reasonable to me. I didn't follow the references in the commit message, but it sounds plausible and probably correct to me. // Martin
On Thu, Feb 9, 2023 at 12:08 PM Martin Storsjö <martin@martin.st> wrote: > > This looks reasonable to me. I didn't follow the references in the commit > message, but it sounds plausible and probably correct to me. > For the record, due to dumb ISO/IEC rule changes regarding how specifications need to be authorized to be free after 2015, it's relatively hard to verify the 14496-12 part. The last version of 14496-12 that was freely available (2015) defined channelcount in AudioSampleEntry as follows: template unsigned int(16) channelcount = 2; Meanwhile the latest 2022 edition defines the related structure(s) as follows: class AudioSampleEntry(codingname) extends SampleEntry (codingname) { const unsigned int(32)[2] reserved = 0; unsigned int(16) channelcount; template unsigned int(16) samplesize = 16; unsigned int(16) pre_defined = 0; const unsigned int(16) reserved = 0 ; template unsigned int(32) samplerate = { default samplerate of media} << 16; // optional boxes follow Box (); // further boxes as needed ChannelLayout(); DownMixInstructions() []; DRCCoefficientsBasic() []; DRCInstructionsBasic() []; DRCCoefficientsUniDRC() []; DRCInstructionsUniDRC() []; // we permit only one DRC Extension box: UniDrcConfigExtension(); // optional boxes follow SamplingRateBox(); ChannelLayout(); } aligned(8) class SamplingRateBox extends FullBox('srat') { unsigned int(32) sampling_rate; } class AudioSampleEntryV1(codingname) extends SampleEntry (codingname) { unsigned int(16) entry_version; // shall be 1, // and shall be in an stsd with version ==1 const unsigned int(16)[3] reserved = 0; template unsigned int(16) channelcount; // shall be correct template unsigned int(16) samplesize = 16; unsigned int(16) pre_defined = 0; const unsigned int(16) reserved = 0 ; template unsigned int(32) samplerate = 1<<16; // optional boxes follow SamplingRateBox(); Box (); // further boxes as needed ChannelLayout(); DownMixInstructions() []; DRCCoefficientsBasic() []; DRCInstructionsBasic() []; DRCCoefficientsUniDRC() []; DRCInstructionsUniDRC() []; // we permit only one DRC Extension box: UniDrcConfigExtension(); // optional boxes follow ChannelLayout(); }
On Thu, Feb 9, 2023 at 9:24 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Thu, Feb 9, 2023 at 12:08 PM Martin Storsjö <martin@martin.st> wrote: > > > > This looks reasonable to me. I didn't follow the references in the commit > > message, but it sounds plausible and probably correct to me. > > > > For the record, due to dumb ISO/IEC rule changes regarding how > specifications need to be authorized to be free after 2015, it's > relatively hard to verify the 14496-12 part. > > The last version of 14496-12 that was freely available (2015) defined > channelcount in AudioSampleEntry as follows: > > template unsigned int(16) channelcount = 2; > > Meanwhile the latest 2022 edition defines the related structure(s) as follows: > > class AudioSampleEntry(codingname) extends SampleEntry (codingname) { > const unsigned int(32)[2] reserved = 0; > unsigned int(16) channelcount; > template unsigned int(16) samplesize = 16; > unsigned int(16) pre_defined = 0; > const unsigned int(16) reserved = 0 ; > template unsigned int(32) samplerate = { default samplerate of media} << 16; > // optional boxes follow > Box (); // further boxes as needed > ChannelLayout(); > DownMixInstructions() []; > DRCCoefficientsBasic() []; > DRCInstructionsBasic() []; > DRCCoefficientsUniDRC() []; > DRCInstructionsUniDRC() []; > // we permit only one DRC Extension box: > UniDrcConfigExtension(); > // optional boxes follow > SamplingRateBox(); > ChannelLayout(); > } > > aligned(8) class SamplingRateBox extends FullBox('srat') { > unsigned int(32) sampling_rate; > } > > class AudioSampleEntryV1(codingname) extends SampleEntry (codingname) { > unsigned int(16) entry_version; // shall be 1, > // and shall be in an stsd with version ==1 > const unsigned int(16)[3] reserved = 0; > template unsigned int(16) channelcount; // shall be correct > template unsigned int(16) samplesize = 16; > unsigned int(16) pre_defined = 0; > const unsigned int(16) reserved = 0 ; > template unsigned int(32) samplerate = 1<<16; > // optional boxes follow > SamplingRateBox(); > Box (); // further boxes as needed > ChannelLayout(); > DownMixInstructions() []; > DRCCoefficientsBasic() []; > DRCInstructionsBasic() []; > DRCCoefficientsUniDRC() []; > DRCInstructionsUniDRC() []; > // we permit only one DRC Extension box: > UniDrcConfigExtension(); > // optional boxes follow > ChannelLayout(); > } Applied to master as 02ddfeadbee52c1ad8c023a93594a8cb957e11db . Jan
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 8d31317838..f0e218e7b7 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1241,13 +1241,7 @@ static int mov_write_audio_tag(AVFormatContext *s, AVIOContext *pb, MOVMuxContex avio_wb16(pb, 16); avio_wb16(pb, track->audio_vbr ? -2 : 0); /* compression ID */ } else { /* reserved for mp4/3gp */ - if (track->par->codec_id == AV_CODEC_ID_FLAC || - track->par->codec_id == AV_CODEC_ID_ALAC || - track->par->codec_id == AV_CODEC_ID_OPUS) { - avio_wb16(pb, track->par->ch_layout.nb_channels); - } else { - avio_wb16(pb, 2); - } + avio_wb16(pb, track->par->ch_layout.nb_channels); if (track->par->codec_id == AV_CODEC_ID_FLAC || track->par->codec_id == AV_CODEC_ID_ALAC) { avio_wb16(pb, track->par->bits_per_raw_sample); diff --git a/tests/ref/fate/mov-mp4-chapters b/tests/ref/fate/mov-mp4-chapters index 34f84de1b1..75cd3b3438 100644 --- a/tests/ref/fate/mov-mp4-chapters +++ b/tests/ref/fate/mov-mp4-chapters @@ -1,4 +1,4 @@ -1fd844c2f5bf77c3344e88e30ad994e1 *tests/data/fate/mov-mp4-chapters.mp4 +7b6aaa99c86fa1f5abfc9f242abcfffa *tests/data/fate/mov-mp4-chapters.mp4 111248 tests/data/fate/mov-mp4-chapters.mp4 #extradata 0: 3469, 0xc6769ddc #tb 0: 1/44100
ISOBMFF (14496-12) made this field ('channelcount') in the AudioSampleEntry structure non-template¹ somewhere before the release of the 2022 edition. As for ETSI TS 126 244 AKA 3GPP file format (V16.1.0, 2020-10), it does not seem contain any references limiting the channelcount entry in AudioSampleEntry or in its own definition of EVSSampleEntry. fate-mov-mp4-chapters test had to be adjusted as it output a mono vorbis stream, which would now be properly marked as such in the container. 1: As per 14496-12: Fields shown as “template” in the box descriptions are fields which are coded with a default value unless a derived specification defines their use and permits writers to use other values than the default. --- libavformat/movenc.c | 8 +------- tests/ref/fate/mov-mp4-chapters | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-)