> On Feb 28, 2023, at 04:24, Jan Ekström <jee...@gmail.com> wrote: > > On Fri, Feb 24, 2023 at 8:29 PM Zhao Zhili <quinkbl...@foxmail.com> wrote: >> >> From: Zhao Zhili <zhiliz...@tencent.com> >> >> It's defined by ISO/IEC 23003-5. >> >> Fixes ticket #10185 >> >> Signed-off-by: Zhao Zhili <zhiliz...@tencent.com> > > Technically if you wanted to split these commits, then this > implementation would have to be limited to one audio channel streams, > as 23003-5:2020 > says as follows: > > - The ChannelLayout as defined in ISO/IEC 14496-12 is mandatory if the > PCM channel count is larger than one.
OK. Will do a ping-pong with v3. > >> --- >> libavformat/movenc.c | 60 +++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 59 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >> index c4fcb5f8b1..3315057b88 100644 >> --- a/libavformat/movenc.c >> +++ b/libavformat/movenc.c >> @@ -1179,6 +1179,47 @@ static int mov_write_btrt_tag(AVIOContext *pb, >> MOVTrack *track) >> return update_size(pb, pos); >> } >> >> +static int is_mp4_pcm_codec(enum AVCodecID codec) >> +{ >> + switch (codec) { >> + case AV_CODEC_ID_PCM_S16BE: >> + case AV_CODEC_ID_PCM_S16LE: >> + case AV_CODEC_ID_PCM_S24BE: >> + case AV_CODEC_ID_PCM_S24LE: >> + case AV_CODEC_ID_PCM_S32BE: >> + case AV_CODEC_ID_PCM_S32LE: >> + >> + case AV_CODEC_ID_PCM_F32BE: >> + case AV_CODEC_ID_PCM_F32LE: >> + case AV_CODEC_ID_PCM_F64BE: >> + case AV_CODEC_ID_PCM_F64LE: >> + return 1; >> + default: >> + return 0; >> + } >> +} > > This should be unnecessary if the check is switched to the codec_tags > in mov_write_audio_tag. > >> + >> +static int mov_write_pcmc_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack >> *track) >> +{ >> + int64_t pos = avio_tell(pb); >> + int format_flags; >> + >> + avio_wb32(pb, 0); /* size */ >> + ffio_wfourcc(pb, "pcmC"); >> + avio_wb32(pb, 0); /* version & flags */ >> + >> + /* 0x01: indicates little-endian format */ >> + format_flags = (track->par->codec_id == AV_CODEC_ID_PCM_F32LE || >> + track->par->codec_id == AV_CODEC_ID_PCM_F64LE || >> + track->par->codec_id == AV_CODEC_ID_PCM_S16LE || >> + track->par->codec_id == AV_CODEC_ID_PCM_S24LE || >> + track->par->codec_id == AV_CODEC_ID_PCM_S32LE); >> + avio_w8(pb, format_flags); >> + avio_w8(pb, track->par->bits_per_raw_sample); >> + >> + return update_size(pb, pos); >> +} > > Generally looks good. I utilized av_get_exact_bits_per_sample, but > since mov_write_audio_tag also writes down bits_per_raw_sample, so I > think being consistent should be OK :) > >> + >> static int mov_write_audio_tag(AVFormatContext *s, AVIOContext *pb, >> MOVMuxContext *mov, MOVTrack *track) >> { >> int64_t pos = avio_tell(pb); >> @@ -1243,7 +1284,8 @@ static int mov_write_audio_tag(AVFormatContext *s, >> AVIOContext *pb, MOVMuxContex >> } else { /* reserved for mp4/3gp */ >> 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) { >> + track->par->codec_id == AV_CODEC_ID_ALAC || >> + is_mp4_pcm_codec(track->par->codec_id)) { > > I know why you think you should be doing this, but: > > 1. 14496-12:2022 still defines AudioSampleEntry::samplesize as > "template unsigned int(16) samplesize = 16;", so ISOBMFF itself only > lets you set 16 here. > 2. 23003-5:2020 does not allow other values to be written (the > downstream spec should explicitly allow writing of non-template > values). This is probably because pcmC itself contains PCM_sample_size > for this same use. > > So writing samplesize here is technically incorrect, even though I > know that most likely some implementations do this (technically > against these specs). Technically it’s against spec. However: 1. It lets old version of libavformat able to demux PCM in mp4. samplesize is used inside mov_parse_stsd_audio for AV_CODEC_ID_PCM. 2. samplesize is a fixed value in spec. If other mp4 demuxer implementations ignore this field, it’s fine. If they use samplesize, it’s better to set the real value. It’s a problem only if they reject samplesize != 16, which is unlikely. What do you think? > > Why channelcount was made non-template yet samplesize was not? A very > good question. Most likely the channel layout v1 and DRC boxes' > changes started requiring the former, but not the latter? > >> avio_wb16(pb, track->par->bits_per_raw_sample); >> } else { >> avio_wb16(pb, 16); >> @@ -1307,6 +1349,8 @@ static int mov_write_audio_tag(AVFormatContext *s, >> AVIOContext *pb, MOVMuxContex >> ret = mov_write_dmlp_tag(s, pb, track); >> else if (track->vos_len > 0) >> ret = mov_write_glbl_tag(pb, track); >> + else if (track->mode == MODE_MP4 && >> is_mp4_pcm_codec(track->par->codec_id)) >> + ret = mov_write_pcmc_tag(s, pb, track); > > It would seem that the generic vos_data extradata writing should be > the last else if in this logic? Could be unnecessary cargo culting, > but at least so far all new codec handling went before it? > > Personally I would have just based this check on the codec tag, a la > > else if (tag == MOV_MP4_IPCM_TAG || tag == MOV_MP4_FPCM_TAG) > // pcmC is required for these tags as per ISO/IEC 23003-5 > ret = mov_write_pcmc_tag(s, pb, track); Will update in v3. > > As that matches the specification's note: > > - Mandatory: Yes, if codingname of SampleEntry is ‘ipcm’ or ‘fpcm’. > > The codec tags are also only defined in codec_mp4_tags, so they would > only get utilized in MP4 and other formats which supposedly support > MP4 formats (ISMV, PSP), so no additional check for that should be > required. > > They already contain stuff such as AV_CODEC_ID_MPEGH_3D_AUDIO, so I > wouldn't consider this a problem :) . Mostly in the sense that if a > more limited set of codecs is wanted for ISMV/PSP, then they should > receive their own more limited listings and not share the mp4 tag > listing :) . > > If you wonder how much the codec tag listing gives you freedoms, you > can check the logic in libavformat/mux.c::validate_codec_tag. > >> >> if (ret < 0) >> return ret; >> @@ -7744,6 +7788,20 @@ static const AVCodecTag codec_mp4_tags[] = { >> { AV_CODEC_ID_MPEGH_3D_AUDIO, MKTAG('m', 'h', 'm', '1') }, >> { AV_CODEC_ID_TTML, MOV_MP4_TTML_TAG }, >> { AV_CODEC_ID_TTML, MOV_ISMV_TTML_TAG }, >> + >> + /* ISO/IEC 23003-5 integer formats */ >> + { AV_CODEC_ID_PCM_S16BE, MKTAG('i', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_S16LE, MKTAG('i', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_S24BE, MKTAG('i', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_S24LE, MKTAG('i', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_S32BE, MKTAG('i', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_S32LE, MKTAG('i', 'p', 'c', 'm') }, >> + /* ISO/IEC 23003-5 floating-point formats */ >> + { AV_CODEC_ID_PCM_F32BE, MKTAG('f', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_F32LE, MKTAG('f', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_F64BE, MKTAG('f', 'p', 'c', 'm') }, >> + { AV_CODEC_ID_PCM_F64LE, MKTAG('f', 'p', 'c', 'm') }, >> + > > This listing seems alright. Personally I added a define for these two > identifiers, but either is fine. > >> { AV_CODEC_ID_NONE, 0 }, >> }; >> #if CONFIG_MP4_MUXER || CONFIG_PSP_MUXER >> -- >> 2.34.1 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".