Hi Hendrik,
On Tue, Dec 21, 2021 at 2:35 PM Hendrik Leppkes <h.lepp...@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 2:11 PM Diederick C. Niehorster
> <dcni...@gmail.com> wrote:
> >
> > Hi Hendrik,
> >
> > Thanks for the review! Comments/questions below:
> >
> > On Tue, Dec 21, 2021 at 1:35 PM Hendrik Leppkes <h.lepp...@gmail.com> wrote:
> > >
> > > On Tue, Dec 21, 2021 at 1:15 PM Diederick Niehorster <dcni...@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...@gmail.com>
> > > > Signed-off-by: Diederick Niehorster <dcni...@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-requ...@ffmpeg.org with subject "unsubscribe".
