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 1E9A04057B for ; Tue, 21 Dec 2021 13:44:34 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3B0BF68AEA0; Tue, 21 Dec 2021 15:44:32 +0200 (EET) Received: from mail-vk1-f176.google.com (mail-vk1-f176.google.com [209.85.221.176]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 986A268AE3E for ; Tue, 21 Dec 2021 15:44:25 +0200 (EET) Received: by mail-vk1-f176.google.com with SMTP id b77so2790940vka.11 for ; Tue, 21 Dec 2021 05:44:25 -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=6QPM1ZIhdhPSQuo4cPyzvXwaQ48OlhMrETZdZQpS/GE=; b=WRSQD6Y/kCTmwGCtpQrexRa/fLQVUf+0fO/4PAnDcvfWLErweHCJ9iJrMFhvmRBplL f5iho3Wnq01iD2OUej0EtrLdlP0475ZyLvqWIT+tbTl9x9/ZNVtd2EoP0eM7uGcMAsIX xb+9Qes9yADm7XouJ9Np7XoyqszOP51eL1TOqXWoWvmEABfiETMxt7rvcAUA89d5Nofr Oi6UVOUq7cgLb1/FgZ65xO+69uZIDhunRAZIsGKGgm0Lvfw9e1XIkG38xNkeD3o6mz8e yIt96z9ifUEACR1T9XMNmE136PO6YBK7ABJbwzoGPApPGdOpqjYqCcruKwnwg+mqC5YE nWzw== 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=6QPM1ZIhdhPSQuo4cPyzvXwaQ48OlhMrETZdZQpS/GE=; b=4ZmUQBZOOXggVUqyTRvmFrSbLvT+XexrpAy0C+FZschHx7vqJu+aNSTMFmUlL0k9xd v+ZlAmsRMwyS6eCZ7zLOLwItguIcs+jZAmAw15wl/jRgMd5ag9qDqdbjDmvA5uYb/DRr 1IFfnF6FyAuaTbEGxklX00hMi6Y4VumdzXjHcCKUAcWJpkVhQoHIzro8S0ntYJeZDPPi 9tibzrdOd2Y9OEdxbBNM/TTslyTZOpvCcl0CAOVIEr2AtDCkCA79gb3DRUlpGrnyniuR sfs8IS7rjvjw+2H1qu5ICW38TkBUbhsP340BtcN/fpzBgDxChE6Nk9AMv1LNWT+aRe2k wSMw== X-Gm-Message-State: AOAM532DE8izxZ5G2bZho8USp6xkgr7nBl4Amg+3h0KUyiCwlBf5OIcY eao1ccUVF/Kf82QwQmLAFneoxQVueE8b1xJMZtVC+rkiDCmdKg== X-Google-Smtp-Source: ABdhPJyRZmWH+z7v/ZuaS/RyRn5HXt8HKIuB4UIIiPnQCGNSa94Pzcg/tyAqm4HgLJuxEm71hgriyKAG/qYnGxX8sLM= X-Received: by 2002:a05:6122:2210:: with SMTP id bb16mr1302415vkb.28.1640094263924; Tue, 21 Dec 2021 05:44:23 -0800 (PST) MIME-Version: 1.0 References: <20211221121239.1201-1-dcnieho@gmail.com> <20211221121239.1201-12-dcnieho@gmail.com> In-Reply-To: From: "Diederick C. Niehorster" Date: Tue, 21 Dec 2021 14:44:06 +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: Hi Hendrik, On Tue, Dec 21, 2021 at 2:35 PM Hendrik Leppkes wrote: > > 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. 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".