Re: [FFmpeg-devel] [PATCH v6 11/12] avdevice/dshow: discover source color range/space/etc

["Diederick C. Niehorster" <dcnieho@gmail.com>]

Hi Hendrik,

Thanks for the review! Comments/questions below:

On Tue, Dec 21, 2021 at 1:35 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 1:15 PM Diederick Niehorster <dcnieho@gmail.com> wrote:
> >
> > Enabled discovering a DirectShow device's color range, space, primaries,
> > transfer characteristics and chroma location, if the device exposes that
> > information. Sets them in the stream's codecpars.
> >
> > Co-authored-by: Valerii Zapodovnikov <val.zapod.vz@gmail.com>
> > Signed-off-by: Diederick Niehorster <dcnieho@gmail.com>
> > ---
> >  libavdevice/dshow.c | 255 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 254 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
> > index 4b1e942c7a..79be32b255 100644
> > --- a/libavdevice/dshow.c
> > +++ b/libavdevice/dshow.c
> > @@ -29,6 +29,31 @@
> >  #include "libavcodec/raw.h"
> >  #include "objidl.h"
> >  #include "shlwapi.h"
> > +// NB: technically, we should include dxva.h and use
> > +// DXVA_ExtendedFormat, but that type is not defined in
> > +// the MinGW headers. The DXVA2_ExtendedFormat and the
> > +// contents of its fields is identical to
> > +// DXVA_ExtendedFormat (see https://docs.microsoft.com/en-us/windows/win32/medfound/extended-color-information#color-space-in-media-types)
> > +// and is provided by MinGW as well, so we use that
> > +// instead. NB also that per the Microsoft docs, the
> > +// lowest 8 bits of the structure, i.e. the SampleFormat
> > +// field, contain AMCONTROL_xxx flags instead of sample
> > +// format information, and should thus not be used.
> > +// NB further that various values in the structure's
> > +// fields (e.g. BT.2020 color space) are not provided
> > +// for either of the DXVA structs, but are provided in
> > +// the flags of the corresponding fields of Media Foundation.
> > +// These may be provided by DirectShow devices (e.g. LAVFilters
> > +// does so). So we use those values here too (the equivalence is
> > +// indicated by Microsoft example code: https://docs.microsoft.com/en-us/windows/win32/api/dxva2api/ns-dxva2api-dxva2_videodesc)
> > +typedef DWORD D3DFORMAT;    // dxva2api.h header needs these types defined before include apparently in WinSDK (not MinGW).
> > +typedef DWORD D3DPOOL;
>
> These come from d3d9types.h, through d3d9.h, which presumably you need
> anyway due to the other D3D9 interfaces used in dxva2api.h, so I'm not
> quite sure when these typedefs are required?

I tried to keep the includes minimal (I don't need any of the d3d9
headers), and to be able to include dxva2api.h, these two typedefs
were needed and nothing else. You prefer i include d3d9types.h instead
of the typedefs?

> > +#include "dxva2api.h"
> > +
> > +#ifndef AMCONTROL_COLORINFO_PRESENT
> > +// not defined in some versions of MinGW's dvdmedia.h
> > +#   define AMCONTROL_COLORINFO_PRESENT 0x00000080 // if set, indicates DXVA color info is present in the upper (24) bits of the dwControlFlags
> > +#endif
> >
> >
> >  static enum AVPixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)
> > @@ -54,6 +79,192 @@ static enum AVPixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)
> >      return avpriv_find_pix_fmt(avpriv_get_raw_pix_fmt_tags(), biCompression); // all others
> >  }
> >
> > +static enum AVColorRange dshow_color_range(DXVA2_ExtendedFormat *fmt_info)
> > +{
> > +    switch (fmt_info->NominalRange)
> > +    {
> > +    case DXVA2_NominalRange_Unknown:
> > +        return AVCOL_RANGE_UNSPECIFIED;
> > +    case DXVA2_NominalRange_Normal: // equal to DXVA2_NominalRange_0_255
> > +        return AVCOL_RANGE_JPEG;
> > +    case DXVA2_NominalRange_Wide:   // equal to DXVA2_NominalRange_16_235
> > +        return AVCOL_RANGE_MPEG;
> > +    case DXVA2_NominalRange_48_208:
> > +        // not an ffmpeg color range
> > +        return AVCOL_RANGE_UNSPECIFIED;
> > +
> > +    // values from MediaFoundation SDK (mfobjects.h)
> > +    case 4:     // MFNominalRange_64_127
> > +        // not an ffmpeg color range
> > +        return AVCOL_RANGE_UNSPECIFIED;
> > +
> > +    default:
> > +        return DXVA2_NominalRange_Unknown;
>
> The default return here should be an AVCOL_RANGE_ constant as well

Absolutely, good catch!

> > +    }
> > +}
> > +
> > +static enum AVColorSpace dshow_color_space(DXVA2_ExtendedFormat *fmt_info)
> > +{
> > +    enum AVColorSpace ret = AVCOL_SPC_UNSPECIFIED;
> > +
> > +    switch (fmt_info->VideoTransferMatrix)
> > +    {
> > +    case DXVA2_VideoTransferMatrix_BT709:
> > +        ret = AVCOL_SPC_BT709;
> > +        break;
> > +    case DXVA2_VideoTransferMatrix_BT601:
> > +        ret = AVCOL_SPC_BT470BG;
> > +        break;
> > +    case DXVA2_VideoTransferMatrix_SMPTE240M:
> > +        ret = AVCOL_SPC_SMPTE240M;
> > +        break;
> > +
> > +    // values from MediaFoundation SDK (mfobjects.h)
> > +    case 4:     // MFVideoTransferMatrix_BT2020_10
> > +    case 5:     // MFVideoTransferMatrix_BT2020_12
> > +        if (fmt_info->VideoTransferFunction==12)    // MFVideoTransFunc_2020_const
>
> Spaces around the == operator please.

ok

> > +            ret = AVCOL_SPC_BT2020_CL;
> > +        else
> > +            ret = AVCOL_SPC_BT2020_NCL;
> > +        break;
> > +    }
> > +
> > +    if (ret == AVCOL_SPC_UNSPECIFIED)
> > +    {
> > +        // if color space not known from transfer matrix,
> > +        // fall back to using primaries to guess color space
>
> Guessing color space from primaries seems unusual, especially since
> the behavior of unknown is actually defined in MSDN docs. IMHO it
> would be better to forward information provided and not try to
> interpret it.

You think i should remove this whole if (ret == AVCOL_SPC_UNSPECIFIED)
code block? I indeed see the docs comment about how
DXVA2_VideoTransferMatrix_Unknown should be interpreted (nothing we
can do here in this function).

> > +        switch (fmt_info->VideoPrimaries)
> > +        {
> > +        case DXVA2_VideoPrimaries_BT709:
> > +            ret = AVCOL_SPC_BT709;
> > +            break;
> > +        case DXVA2_VideoPrimaries_BT470_2_SysM:
> > +            ret = AVCOL_SPC_FCC;
> > +            break;
> > +        case DXVA2_VideoPrimaries_BT470_2_SysBG:
> > +        case DXVA2_VideoPrimaries_EBU3213:   // this is PAL
> > +            ret = AVCOL_SPC_BT470BG;
> > +            break;
> > +        case DXVA2_VideoPrimaries_SMPTE170M:
> > +        case DXVA2_VideoPrimaries_SMPTE_C:
> > +            ret = AVCOL_SPC_SMPTE170M;
> > +            break;
> > +        case DXVA2_VideoPrimaries_SMPTE240M:
> > +            ret = AVCOL_SPC_SMPTE240M;
> > +            break;
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".