[FFmpeg-devel] [PATCH] avcodec/dpxenc: stop hardcoding color trc/primaries

[Paul B Mahol]

On Thu, Oct 08, 2020 at 12:27:02PM +0100, Harry Mallon wrote:
> 
> 
> > On 7 Oct 2020, at 22:02, Paul B Mahol <onemda at gmail.com> wrote:
> > 
> > Signed-off-by: Paul B Mahol <onemda at gmail.com>
> > ---
> > libavcodec/dpxenc.c | 36 ++++++++++++++++++++++++++++++++++--
> > tests/ref/lavf/dpx  |  2 +-
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/dpxenc.c b/libavcodec/dpxenc.c
> > index a5960334d5..56840a8d33 100644
> > --- a/libavcodec/dpxenc.c
> > +++ b/libavcodec/dpxenc.c
> > @@ -173,6 +173,38 @@ static void encode_gbrp12(AVCodecContext *avctx, const AVFrame *pic, uint16_t *d
> >     }
> > }
> > 
> > +static int get_dpx_pri(int color_pri)
> > +{
> > +    switch (color_pri) {
> > +    case AVCOL_PRI_BT709:
> > +        return 6;
> > +    case AVCOL_PRI_SMPTE240M:
> > +    case AVCOL_PRI_SMPTE170M:
> > +        return 9;
> 
> I think perhaps this should be 8 (ITU 601 525), rather than 9 (Composite video SMPTE 170M), but I am not sure?

The smpte170m is explicitly mention in specification, so make sure you use latest version of it.

> 
> > +    case AVCOL_PRI_BT470BG:
> > +        return 10;
> 
> Perhaps this should be 7 (ITU 601 625), rather than 10 (ITU 624-4 Composite video PAL), again not sure which is most widely used?

see first comment.

> 
> > +    default:
> > +        return 0;
> > +    }
> > +}
> 
> In DPX files containing colour difference information the colorspace would also be keyed off the value returned from this function. Perhaps it should be taken into account here (for files containing YCbCr)?

Sorry, this does not make sense, the color_prim/trc is meaningfull for both yuv and rgb.

> 
> > +
> > +static int get_dpx_trc(int color_trc)
> > +{
> > +    switch (color_trc) {
> > +    case AVCOL_TRC_LINEAR:
> > +        return 2;
> > +    case AVCOL_TRC_BT709:
> > +        return 6;
> > +    case AVCOL_TRC_SMPTE240M:
> > +    case AVCOL_TRC_SMPTE170M:
> > +        return 9;
> 
> This value could be 7, 8 or 9. From my reading of the spec the best might be to take colour_primaries into account and do something like:
> 
> if (AVCOL_PRI_BT470BG) return 7 (ITU 601 625)
> else return 8 (ITU 601 525)
> 

see first comment.

> > +    case AVCOL_TRC_GAMMA22:
> > +        return 10;
> 
> 10 is ITU 624-4 Composite video PAL, which says it has gamma = 2.8 (AVCOL_TRC_GAMMA28). https://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BT.624-4-1990-PDF-E.pdf
> 

Hmm i will double check.

> > +    default:
> > +        return 0;
> > +    }
> > +}
> > +
> > 
> > [..]
> > 
> 
> Best,
> Harry
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".