diff mbox

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

Message ID 20170829001321.1319-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 29, 2017, 12:13 a.m. UTC 
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mxfenc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

wm4 Aug. 29, 2017, 9:02 a.m. UTC | #1 
On Tue, 29 Aug 2017 02:13:19 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Signed-off-by: Michael Niedermayer <michael@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?
Paul B Mahol Aug. 29, 2017, 9:05 a.m. UTC | #2 
On 8/29/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Signed-off-by: Michael Niedermayer <michael@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"},\

Above change is unneeded.

> +    { "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"},\
>
>
>
> --
> 2.14.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Aug. 29, 2017, 9:45 a.m. UTC | #3 
On Tue, Aug 29, 2017 at 11:05:25AM +0200, Paul B Mahol wrote:
> On 8/29/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Signed-off-by: Michael Niedermayer <michael@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"},\
> 
> Above change is unneeded.

The multiline macro needs the added \
if more is added at the end
I could add the new option at the top if you prefer ?

[...]
Michael Niedermayer Aug. 29, 2017, 9:59 a.m. UTC | #4 
On Tue, Aug 29, 2017 at 11:02:32AM +0200, wm4 wrote:
> On Tue, 29 Aug 2017 02:13:19 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@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

[...]
wm4 Aug. 29, 2017, 10:02 a.m. UTC | #5 
On Tue, 29 Aug 2017 11:59:05 +0200
Michael Niedermayer <michael@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@niedermayer.cc> wrote:
> >   
> > > Signed-off-by: Michael Niedermayer <michael@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.
Michael Niedermayer Aug. 29, 2017, 1:02 p.m. UTC | #6 
On Tue, Aug 29, 2017 at 12:02:18PM +0200, wm4 wrote:
> On Tue, 29 Aug 2017 11:59:05 +0200
> Michael Niedermayer <michael@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@niedermayer.cc> wrote:
> > >   
> > > > Signed-off-by: Michael Niedermayer <michael@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.

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

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.

Thanks

[...]
wm4 Aug. 29, 2017, 2:30 p.m. UTC | #7 
On Tue, 29 Aug 2017 15:02:49 +0200
Michael Niedermayer <michael@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@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@niedermayer.cc> wrote:
> > > >     
> > > > > Signed-off-by: Michael Niedermayer <michael@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.
diff mbox

Patch

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"},\