Re: [FFmpeg-devel] [PATCH v2 1/8] avformat/movenc: add PCM in mp4 support

["\"zhilizhao(赵志立)\"" <quinkblack@foxmail.com>]

> On Feb 28, 2023, at 04:24, Jan Ekström <jeebjp@gmail.com> wrote:
> 
> On Fri, Feb 24, 2023 at 8:29 PM Zhao Zhili <quinkblack@foxmail.com> wrote:
>> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> It's defined by ISO/IEC 23003-5.
>> 
>> Fixes ticket #10185
>> 
>> Signed-off-by: Zhao Zhili <zhilizhao@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-request@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-request@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-request@ffmpeg.org with subject "unsubscribe".