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

Hendrik Leppkes h.leppkes at gmail.com
Tue Dec 21 15:35:09 EET 2021


On Tue, Dec 21, 2021 at 2:11 PM Diederick C. Niehorster
<dcnieho at gmail.com> wrote:
>
> Hi Hendrik,
>
> Thanks for the review! Comments/questions below:
>
> On Tue, Dec 21, 2021 at 1:35 PM Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >
> > On Tue, Dec 21, 2021 at 1:15 PM Diederick Niehorster <dcnieho at 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 at gmail.com>
> > > Signed-off-by: Diederick Niehorster <dcnieho at 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?

Definitely prefer including a header for those types then re-defining
them, yes. That can get a bit out of hand quickly otherwise. And these
older Windows headers are also well defined, so there shouldn't be any
issues.

>
> > > +#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).

Thats what I would've suggested, but I don't know if you had that in
there for any specific cases with capture hardware or the likes.

>
> > > +        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 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".


More information about the ffmpeg-devel mailing list