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 3BAA140564 for ; Tue, 21 Dec 2021 13:11:42 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1839468AE9B; Tue, 21 Dec 2021 15:11:40 +0200 (EET) Received: from mail-vk1-f181.google.com (mail-vk1-f181.google.com [209.85.221.181]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8F7F268AE9B for ; Tue, 21 Dec 2021 15:11:33 +0200 (EET) Received: by mail-vk1-f181.google.com with SMTP id b77so2729088vka.11 for ; Tue, 21 Dec 2021 05:11:33 -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=qq3c9FWnwVZFgUXXm/C3f3UyxKF3hj9Un6Q30iQdOVY=; b=HNT52NEA3xpMoSxa3FqKARI0wI2MVJgCDkRswtKhFEwo7q+RwWvxUEfa9kxrZwMELE mX4jXR8K00mc3+aJ8XDJY29bCywWBWC9q8Taui86lq25A9Yn9znaN4GtxkRZhu6op+TP py6X9vVCaAAHo4TlKQBVeAzIcvTgAoHOs8CciyJMb37fBjecRiLl0eYUdQQvs4apPMWs FEWIx9fL06NYYqx2A/0Dm+rig8ju1BL4RjSz30vBz5tXh+uJoxU0ULghB0HFBj9KmoDb 45Fw5pmGn5jFoia8yiS9WCrKnSnKO44mZlk9ssqf/S/9vMNNZCSgTYMNwPmWdi28sH1O JJNw== 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=qq3c9FWnwVZFgUXXm/C3f3UyxKF3hj9Un6Q30iQdOVY=; b=0z/huZbfuhuLH6VWmd14E/RFpbxOeZDtV5Qr3/qY79SmDx7Qa05IttbarXqpvCKZAg FCZ1xLnXpFA+1A2e9dKrhmYV6MlUU4tkPhTyEfJJ3I9bDCodoejbgdAKDg+VU0UG86Yn ofHdb5vKAAz2JoH/fY7RPNmDtFPP8AsXRLUbRGfw1UEHawZZmDbRWIepPLQKd1SYjoNS CQg4OstmDYMRewvk9eENuzeXbmBRid0pLxjlqu0+djhk15ZGSf4ES/+4bFBrvouF6OMB IA/LthrVmUF2jFZ5Bnv1D14/susvb/A3kRwHNuH+0TlC3B47aw0gDSIk77uFGRZ9kmX4 Di8g== X-Gm-Message-State: AOAM531jxR0x4CSi/auzWTipwjxhmN97mSuNieqvQAp9a2ee5rb2Zl5i YDc7mHCIFX37e6qQ9sVEOge2f38gA1pj2tcodYDmuw1lnlvwEw== X-Google-Smtp-Source: ABdhPJxOL7t9LM2XYpf/dV/lPDn5ddK1vLWuqGTHTylag9R3tWfOzJ4kUiwLICTZ6Umgt5MEOYHdiujrkxR2eTplurM= X-Received: by 2002:a1f:3f11:: with SMTP id m17mr1123448vka.11.1640092291704; Tue, 21 Dec 2021 05:11:31 -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:11:14 +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, 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? > > +#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). > > + 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".