From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 2656940577 for ; Tue, 21 Dec 2021 13:35:30 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6E2DA68AF2C; Tue, 21 Dec 2021 15:35:29 +0200 (EET) Received: from mail-yb1-f177.google.com (mail-yb1-f177.google.com [209.85.219.177]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3F03468AE9B for ; Tue, 21 Dec 2021 15:35:23 +0200 (EET) Received: by mail-yb1-f177.google.com with SMTP id d10so38910283ybe.3 for ; Tue, 21 Dec 2021 05:35:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=AjkdGL8ICpFBgQJ5BIIakFv6qj8uTwlDRMCM89qmOAY=; b=CC5dXBgvngfiT/q9KuoRWnrBT2YMFN1fRNmX1zWEbzlpb5k2EkFZei8bUJ8q3LYs3p RfSbePrpKiSky5B5CBRi1FKSGclaI/4tdYumo8Umk8y6hJ8epSE58qtayN8FcAbs7fre cWYJb9RWHCy3bfVCRc+SGTV4FVGGUEgRlhXWUilqFg/5fXxCnpL8dRiFUwXFVbw8UzPk sexGsxzM/xdvsdaHpPdka/uogBeE377UYhwQXDafO8RjbVU/i40E0JVmBrlVT5y2dU7d GV5ARMcK3Wb0Up68GH7ur4QfpK/MyeZchn6cXBw3pINsSfFzs0GVC3NJ5Aq4yPqx7QV5 l2rQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=AjkdGL8ICpFBgQJ5BIIakFv6qj8uTwlDRMCM89qmOAY=; b=fBJnl6E+rgxfIjmA2/a1UIzOrTYi2wOpX86NswaHItQ0VCK6VYybkDAvkKVaYQyaOe fh1ti0YyJAyb9rPnYUG+PED48YD+CL3VMKlSaHpFrZ4Mx9rOIYqBoDU/6ciU+4TAsKw+ lwsY1ScYMMvQdGtP418lcL27UfgyHrVSVPjIxOWeVprZ7CM1fxKgY4D/7/i2/TIFUtgc O+xAKGbJ5/rprD7D736exI5n2gh5KDeKKfduxlfiCXfNJy1KF/3ldEnIPcyFiBW6HG4V tLMq/vgqiv2Wxduw0IdhEutHqxr5MC+0jDdwG57piC5uXZ73eutM27X8z5EtALrGZCuc Ny+g== X-Gm-Message-State: AOAM531srnz9f5v50pt8N3IvCPqS6Hzq9RGA1w14Qj2cUqSaFYEZ8B0Q iAzHK9aj4iyeTMYdgtG2wo+mt5Wgdt0sKSlmaTXWYzLg X-Google-Smtp-Source: ABdhPJwOd3RnCcRaWdh5dcwiTHnpJIrAGXCTwP07yV/ZxSaXEfh4Asfqt9v8POApyDwLID4s1KBciPYoYOb00HtizSw= X-Received: by 2002:a05:6902:110d:: with SMTP id o13mr4719364ybu.715.1640093721210; Tue, 21 Dec 2021 05:35:21 -0800 (PST) MIME-Version: 1.0 References: <20211221121239.1201-1-dcnieho@gmail.com> <20211221121239.1201-12-dcnieho@gmail.com> In-Reply-To: From: Hendrik Leppkes Date: Tue, 21 Dec 2021 14:35:09 +0100 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH v6 11/12] avdevice/dshow: discover source color range/space/etc X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Tue, Dec 21, 2021 at 2:11 PM Diederick C. Niehorster wrote: > > Hi Hendrik, > > Thanks for the review! Comments/questions below: > > On Tue, Dec 21, 2021 at 1:35 PM Hendrik Leppkes wrote: > > > > On Tue, Dec 21, 2021 at 1:15 PM Diederick Niehorster 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 > > > Signed-off-by: Diederick Niehorster > > > --- > > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ 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".