[FFmpeg-devel] [PATCH v2 1/8] avformat/movenc: add PCM in mp4 support
"zhilizhao(赵志立)"
quinkblack at foxmail.com
Tue Feb 28 06:41:22 EET 2023
> On Feb 28, 2023, at 04:24, Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Fri, Feb 24, 2023 at 8:29 PM Zhao Zhili <quinkblack at foxmail.com> wrote:
>>
>> From: Zhao Zhili <zhilizhao at tencent.com>
>>
>> It's defined by ISO/IEC 23003-5.
>>
>> Fixes ticket #10185
>>
>> Signed-off-by: Zhao Zhili <zhilizhao at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list