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

["Jan Ekström" <jeebjp@gmail.com>]

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