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

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

Hi Hendrik,

On Tue, Dec 21, 2021 at 2:35 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 2:11 PM Diederick C. Niehorster
> <dcnieho@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@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?
>
> 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.

Ok, I'll replace with an include of d3d9types.h

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

No, no specific case this fixes. I'll remove.

Thanks,
Dee
_______________________________________________
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".