[FFmpeg-devel] [PATCH 1/3] avformat/mxfenc: Allow overriding numerical color_siting value.

[wm4]

On Tue, 29 Aug 2017 15:02:49 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Tue, Aug 29, 2017 at 12:02:18PM +0200, wm4 wrote:
> > On Tue, 29 Aug 2017 11:59:05 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Tue, Aug 29, 2017 at 11:02:32AM +0200, wm4 wrote:  
> > > > On Tue, 29 Aug 2017 02:13:19 +0200
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libavformat/mxfenc.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > > > index 12fc9abbc6..ccfa0d6341 100644
> > > > > --- a/libavformat/mxfenc.c
> > > > > +++ b/libavformat/mxfenc.c
> > > > > @@ -322,6 +322,7 @@ typedef struct MXFContext {
> > > > >      uint8_t umid[16];        ///< unique material identifier
> > > > >      int channel_count;
> > > > >      int signal_standard;
> > > > > +    int color_siting;
> > > > >      uint32_t tagged_value_count;
> > > > >      AVRational audio_edit_rate;
> > > > >      int store_user_comments;
> > > > > @@ -2085,6 +2086,8 @@ static int mxf_write_header(AVFormatContext *s)
> > > > >              case AVCHROMA_LOC_TOP:     sc->color_siting = 1; break;
> > > > >              case AVCHROMA_LOC_CENTER:  sc->color_siting = 3; break;
> > > > >              }
> > > > > +            if (mxf->color_siting >= 0)
> > > > > +                sc->color_siting = mxf->color_siting;
> > > > >  
> > > > >              mxf->timecode_base = (tbc.den + tbc.num/2) / tbc.num;
> > > > >              spf = ff_mxf_get_samples_per_frame(s, tbc);
> > > > > @@ -2668,7 +2671,9 @@ static int mxf_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int
> > > > >      { "smpte349m", "SMPTE 349M (1485 Mbps mappings)",\
> > > > >        0, AV_OPT_TYPE_CONST, {.i64 = 6}, -1, 7, AV_OPT_FLAG_ENCODING_PARAM, "signal_standard"},\
> > > > >      { "smpte428", "SMPTE 428-1 DCDM",\
> > > > > -      0, AV_OPT_TYPE_CONST, {.i64 = 7}, -1, 7, AV_OPT_FLAG_ENCODING_PARAM, "signal_standard"},
> > > > > +      0, AV_OPT_TYPE_CONST, {.i64 = 7}, -1, 7, AV_OPT_FLAG_ENCODING_PARAM, "signal_standard"},\
> > > > > +    { "color_siting", "Force/set Color siting",\
> > > > > +      offsetof(MXFContext, color_siting), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 7, AV_OPT_FLAG_ENCODING_PARAM, "color_siting"},\
> > > > >  
> > > > >  
> > > > >      
> > > > 
> > > > What an absurd patch. This should be done by generic code overriding
> > > > the chroma_location field on AVFrames sent to the encoders. We don't
> > > > need inconsistent private options for an inconsistent set of encoders
> > > > to override generic parameters in an inconsistent way. What's even the
> > > > point?    
> > > 
> > > The point of this change is to be able to set any color siting value.
> > > MXF has some redundant values. The fields from AVFrame are not enough
> > > to choose this unless we want MXF specific information in AVFrames
> > > 
> > > [...]  
> > 
> > You probably want to use some specialized MXF tool then.  
> 
> It should be possible to generate mxf files with just one tool not 2.
> In fact this change is for someone else not me, i probably wont be
> using it much.

MXF is such a complex format that FFmpeg can't hope it can generate all
possible MXF variants and use all of its possible features. There are
plenty of mp4/mkv/other features that FFmpeg can't write. So that's no
justification on its own.

I could probably send 100s of patches adding tiny unneeded features for
writing obscure mkv elements using private options.

Sometimes, such features have a real justification because either a
feature doesn't fit into FFmpeg's abstractions, or because it's needed
as a workaround to deal with multiple broken MXF readers, but I've not
seen that from you either.

> Do you object to this patch ?
> If you veto it, i will not apply it of course.

Yes.

> 
> if not i will apply it with the lines reordered to avoid the \ issue
> raised by paul

> as FFmpeg is intended to be a universal multimedia tool and requiring
> a 2nd tool is cumbersome.

Oh, that's a great reason to dump unneeded crap into FFmpeg.