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

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>
---
 libavformat/movenc.c | 60 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

lör 2023-02-25 klockan 02:28 +0800 skrev Zhao Zhili:
> 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>
> ---
>  libavformat/movenc.c | 60
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)

Looks OK

/Tomas

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.

> ---
>  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).

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);

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".

> 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".