[FFmpeg-devel] [PATCH] set/force channelcount in MXF D-10

[Matthieu Bouron]

On Tue, Jun 24, 2014 at 6:53 PM, Gaullier Nicolas <
nicolas.gaullier at arkena.com> wrote:

> There are interoperability issues with D-10 related to the channelcount
> property in the generic sound essence descriptor.
>
> On one side, SMPTE 386M requires channel count to be 4 or 8, other values
> being prohibited.
> The most widespread value is 8, which seems straightforward as it is the
> actual size of the allocated structure/disk space.
> At the end, it appears that some vendors or workflows do require this
> descriptor to be 8, and otherwise just "fail".
>
> On the other side, at least AVID and ffmpeg do write/set the channel count
> to the exact number of channels really "used",
> usually 2 or 4, or any other value. And on the decoding side, ffmpeg (for
> example) make use of the channel count for probing
> and only expose this limited number of audio streams
> (which make sense but has strong impact on ffmpeg command line usage,
> output, and downstream workflow).
>
> At the end, I find it pretty usefull to simply give ffmpeg the ability to
> force/set the channel count to any value the user wants.
> (there are turnaround using complex filters, pans, amerge etc., but it is
> quite boring and requires the command line to be adapted to the input file
> properties)
>
> -------
> Nicolas
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 9572623..d156e74 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -294,6 +294,7 @@ typedef struct MXFContext {
>      uint64_t body_offset;
>      uint32_t instance_number;
>      uint8_t umid[16];        ///< unique material identifier
> +    int channel_count;
>  } MXFContext;
>
>  static const uint8_t uuid_base[]            = {
> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd };
> @@ -996,6 +997,7 @@ static void mxf_write_mpegvideo_desc(AVFormatContext
> *s, AVStream *st)
>  static void mxf_write_generic_sound_common(AVFormatContext *s, AVStream
> *st, const UID key, unsigned size)
>  {
>      AVIOContext *pb = s->pb;
> +    MXFContext *mxf = s->priv_data;
>
>      mxf_write_generic_desc(s, st, key, size+5+12+8+8);
>
> @@ -1009,7 +1011,15 @@ static void
> mxf_write_generic_sound_common(AVFormatContext *s, AVStream *st, con
>      avio_wb32(pb, 1);
>
>      mxf_write_local_tag(pb, 4, 0x3D07);
> -    avio_wb32(pb, st->codec->channels);
> +    if ((mxf->channel_count == -1) || (mxf->channel_count ==
> st->codec->channels)) {

+        avio_wb32(pb, st->codec->channels);
> +    } else if (s->oformat == &ff_mxf_d10_muxer) {
> +        avio_wb32(pb, mxf->channel_count);
> +    } else {
> +        av_log(s, AV_LOG_ERROR, "mxf_channelcount requires MXF D-10 and
> will be ignored\n");
> +        avio_wb32(pb, st->codec->channels);
> +    }
> +
>
>      mxf_write_local_tag(pb, 4, 0x3D01);
>      avio_wb32(pb, av_get_bits_per_sample(st->codec->codec_id));
> @@ -2156,6 +2166,19 @@ static int mxf_interleave(AVFormatContext *s,
> AVPacket *out, AVPacket *pkt, int
>                                 mxf_interleave_get_packet,
> mxf_compare_timestamps);
>  }
>
> +static const AVOption d10_options[] = {
> +    { "mxf_channelcount", "Force/set channelcount in generic sound
> essence descriptor",
> +      offsetof(MXFContext, channel_count), AV_OPT_TYPE_INT, {.i64 = -1},
> -1, 8, AV_OPT_FLAG_ENCODING_PARAM},
> +    { NULL },
> +};
> +
> +static const AVClass mxf_d10_muxer_class = {
> +    .class_name     = "MXF-D10 muxer",
> +    .item_name      = av_default_item_name,
> +    .option         = d10_options,
> +    .version        = LIBAVUTIL_VERSION_INT,
> +};
> +
>  AVOutputFormat ff_mxf_muxer = {
>      .name              = "mxf",
>      .long_name         = NULL_IF_CONFIG_SMALL("MXF (Material eXchange
> Format)"),
> @@ -2183,4 +2206,5 @@ AVOutputFormat ff_mxf_d10_muxer = {
>      .write_trailer     = mxf_write_footer,
>      .flags             = AVFMT_NOTIMESTAMPS,
>      .interleave_packet = mxf_interleave,
> +    .priv_class        = &mxf_d10_muxer_class,
>  };
>

Hello Nicolas,

First of all, thanks for your patch.

It seems good to me however I would add some check on the channel_count
value:
  - to restrict it to 4 or 8 depending on the audio format (since the
option is tied to the D10 spec), or at least warn the user.
  - warn the user if (channel_count < st->codec->channels) that some stream
will be discarded.

What do you think ?

Best regards,
Matthieu