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

[Matthieu Bouron]

On Wed, Jun 25, 2014 at 11:29 AM, Gaullier Nicolas <
nicolas.gaullier at arkena.com> wrote:

> >De : ffmpeg-devel-bounces at ffmpeg.org [mailto:
> ffmpeg-devel-bounces at ffmpeg.org] De la part de 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
> >
> >
> >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 ?
>
> Indeed !
> I think restriction is a bit risky as (sadly) it appears some
> vendors/integrators do make use of incompliant channel counts : I had taken
> the most conservative approach of "warning + inform about this new option".
> Concerning the case "channel_count < st->codec->channels", I now warn that
> some channels are discarded, but the processing remains unchanged : the
> data are actually preserved, and at the end, only the decoder behavior will
> really make them still available or not.
> Note: the warnings now only show once when writing the header/preface the
> first time (I realized that the warnings showed twice because of header
> metadata refresh when writing the footer).
>
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 9572623..6e8ac13 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,8 @@ 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;
> +    int show_warnings = !mxf->footer_partition_offset;
>
>      mxf_write_generic_desc(s, st, key, size+5+12+8+8);
>
> @@ -1009,7 +1012,21 @@ 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) {
> +        if (show_warnings && (s->oformat == &ff_mxf_d10_muxer) &&
> (st->codec->channels != 4) && (st->codec->channels != 8))
> +            av_log(s, AV_LOG_WARNING, "the number of audio channels shall
> be 4 or 8 : the output will not comply to MXF D-10 specs, use
> -mxf_channelcount to fix this\n");
> +        avio_wb32(pb, st->codec->channels);
> +    } else if (s->oformat == &ff_mxf_d10_muxer) {
> +        if (show_warnings && (mxf->channel_count < st->codec->channels))
> +            av_log(s, AV_LOG_WARNING, "mxf_channelcount < actual number
> of audio channels : some channels will be discarded\n");
> +        if (show_warnings && (mxf->channel_count != 4) &&
> (mxf->channel_count != 8))
> +            av_log(s, AV_LOG_WARNING, "mxf_channelcount shall be set to 4
> or 8 : the output will not comply to MXF D-10 specs\n");
> +        avio_wb32(pb, mxf->channel_count);
> +    } else {
> +        if (show_warnings)
> +            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 +2173,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 +2213,5 @@ AVOutputFormat ff_mxf_d10_muxer = {
>      .write_trailer     = mxf_write_footer,
>      .flags             = AVFMT_NOTIMESTAMPS,
>      .interleave_packet = mxf_interleave,
> +    .priv_class        = &mxf_d10_muxer_class,
>  };
>

The patch looks good to me.

Thanks,
Matthieu