From d2a0f157ab728fd37e16804e557e04755afd76f9 Mon Sep 17 00:00:00 2001 From: HowardsPlayPen Date: Fri, 30 Apr 2021 13:27:53 +0100 Subject: [PATCH] Annotated a number of issues I found Added some descriptions of the issues I found or what confused me - hopefully this will help anyone else that also found this code as I did. --- Media.cpp | 31 ++++++++++++++----- Media.h | 2 +- README.md | 15 ++++++++- main.cpp | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 129 insertions(+), 10 deletions(-) diff --git a/Media.cpp b/Media.cpp index baaa75f..4f10401 100644 --- a/Media.cpp +++ b/Media.cpp @@ -78,7 +78,7 @@ HRESULT Media::CreateCaptureDevice() is selected */ //Get a source reader from the first available device - SetSourceReader(devices[0]); + hr = SetSourceReader(devices[0]); WCHAR *nameString = NULL; // Get the human-friendly name of the device @@ -89,6 +89,10 @@ HRESULT Media::CreateCaptureDevice() if (SUCCEEDED(hr)) { + /// Note: See https://docs.microsoft.com/en-us/windows/win32/medfound/recommended-8-bit-yuv-formats-for-video-rendering + /// for a good description of the formats. + /// On my computer the webcam support YUY2 which has a stride of 2 + //allocate a byte buffer for the raw pixel data bytesPerPixel = abs(stride) / width; rawData = new BYTE[width*height * bytesPerPixel]; @@ -112,6 +116,9 @@ HRESULT Media::SetSourceReader(IMFActivate *device) EnterCriticalSection(&criticalSection); + // Note: I have found the following call to fail with the error: + // hr 0xc00d36b4 : The data specified for the media type is invalid, inconsistent, or not supported by this object. HRESULT + // So some vidcap devices are not happy with this combination. hr = device->ActivateObject(__uuidof(IMFMediaSource), (void**)&source); //get symbolic link for the device @@ -132,19 +139,28 @@ HRESULT Media::SetSourceReader(IMFActivate *device) // Try to find a suitable output type. if (SUCCEEDED(hr)) { + /// Update: This for loop was aimed at iterating across all supported (native) media types - but there were a couple of strange things which have been updated below for (DWORD i = 0; ; i++) { hr = sourceReader->GetNativeMediaType((DWORD)MF_SOURCE_READER_FIRST_VIDEO_STREAM,i,&mediaType); + + /// Note: FAILED() here does not stop on S_FALSE - which is correct, as it should continue in this loop but might not sound logical + /// but if you loop through ALL supported media types the GetNativeMediaType will eventually return a code to indicate there are no more types available if (FAILED(hr)) { break; } + // Note: I moved this call to here as elsewhere in this code it uses stride without checking if it was zero + //Get the stride for this format so we can calculate the number of bytes per pixel + GetDefaultStride(mediaType, &stride); + hr = IsMediaTypeSupported(mediaType); - if (FAILED(hr)) { break; } + if (FAILED(hr)) { break; } /// Note: this is not detecting S_FALSE //Get width and height MFGetAttributeSize(mediaType, MF_MT_FRAME_SIZE, &width, &height); if (mediaType) { mediaType->Release(); mediaType = NULL; } - if (SUCCEEDED(hr))// Found an output type. + /// Note: this SUCCESS(hr) is WRONG - i.e. if 'hr' was S_FALSE (did not find media type) then this still goes ahead thinking it HAD found an output type + if (S_OK == hr)// Found an output type - by explicityly checking for S_OK break; } } @@ -177,9 +193,6 @@ HRESULT Media::IsMediaTypeSupported(IMFMediaType *pType) BOOL bFound = FALSE; GUID subtype = { 0 }; - //Get the stride for this format so we can calculate the number of bytes per pixel - GetDefaultStride(pType, &stride); - if (FAILED(hr)) { return hr; } hr = pType->GetGUID(MF_MT_SUBTYPE, &subtype); @@ -187,10 +200,14 @@ HRESULT Media::IsMediaTypeSupported(IMFMediaType *pType) if (FAILED(hr)) {return hr; } + /// Update: The below is still lying - eg NV12 or RGB32 are not supported as there is no NV12 to BGRA function in main.cpp if (subtype == MFVideoFormat_RGB32 || subtype == MFVideoFormat_RGB24 || subtype == MFVideoFormat_YUY2 || subtype == MFVideoFormat_NV12) return S_OK; else - return S_FALSE; + /// Update: Returning S_FALSE here is unclear as SUCCESS(S_FALSE) is actually true + /// See https://docs.microsoft.com/en-us/windows/win32/learnwin32/error-handling-in-com + /// So either return S_FALSE and check for if(S_FALSE == hr) specifically, but not if you use SUCCESS() or FAIL() as was done in this wider code + return S_FALSE; return hr; } diff --git a/Media.h b/Media.h index f91b6c6..de1a327 100644 --- a/Media.h +++ b/Media.h @@ -30,7 +30,7 @@ class Media : public IMFSourceReaderCallback //this class inhertis from IMFSourc public: LONG stride; int bytesPerPixel; - GUID videoFormat; + GUID videoFormat; // MFVideoFormat_RGB32 / MFVideoFormat_RGB24 / MFVideoFormat_YUY2 / MFVideoFormat_NV12 UINT height; UINT width; WCHAR deviceNameString[2048]; diff --git a/README.md b/README.md index 3fb1894..4f71020 100644 --- a/README.md +++ b/README.md @@ -4,4 +4,17 @@ and was a good introduction to accessing Webcams using Microsoft Foundation api So I thought I would update and annotate the code to show what I found. -This first commit is the base code as was downloaded - ie. prior to any updates and annotations. +1) RGB vs YUY2 NV12 etc + +Inside Media::IsMediaTypeSupported it checks if the native media type is one of a list of options, including YUY2 and NV12 BUT it then makes a gross assumption when displaying the pixel data that it is RGB. As others have seen this basically means it will exception nastily (both my webcams natively support YUY2 and so the code did not work out of the box) + +1a) I found Media::IsMediaTypeSupported confusing + +It returned S_OK for formats that it clearly did not support (as it only supported RGB24). +It returned S_FALSE for everything else - but in Media::SetSourceReader where it called IsMediaSupported it checks for FAILED on this return code (but S_FALSE is still a success in COM speak) + +IT ALSO then checks for SUCCESS(hr) to see if it had found a supported media type - but as above it would think SUCCESS(S_FALSE) had worked (!!). So it would stop looping even though it had not found a supported type! + +2) I added an implementation of YUY2 to RGBA (TransformImage_YUY2) + +I merely referred to an implementation on the Intel website https://software.intel.com/sites/landingpage/mmsf/documentation/preview_8cpp.html#af3d9de67be8e955b824d4b497bba4c96 \ No newline at end of file diff --git a/main.cpp b/main.cpp index bc511f9..e6d561f 100644 --- a/main.cpp +++ b/main.cpp @@ -3,6 +3,8 @@ #include #include +#include + struct BGRAPixel { BYTE b; @@ -27,6 +29,69 @@ void RGB24_to_BGRA32(BGRAPixel* destinationBuffer, const BYTE* sourceBuffer, DWO } } + +__forceinline BYTE Clip(int clr) +{ + return (BYTE)(clr < 0 ? 0 : (clr > 255 ? 255 : clr)); +} + +BGRAPixel ConvertYCrCbToRGB(int y, + int cr, + int cb +) +{ + BGRAPixel rgbq; + + int c = y - 16; + int d = cb - 128; + int e = cr - 128; + + rgbq.r = Clip((298 * c + 409 * e + 128) >> 8); + rgbq.g = Clip((298 * c - 100 * d - 208 * e + 128) >> 8); + rgbq.b = Clip((298 * c + 516 * d + 128) >> 8); + + return rgbq; +} + +/// Note: this is taken from https://software.intel.com/sites/landingpage/mmsf/documentation/preview_8cpp.html#af3d9de67be8e955b824d4b497bba4c96 +/// In the online version (from Intel) the comment below on Byte order was incorrect - although the code was fine (!). Never trust documentation..(?) +void TransformImage_YUY2(BYTE* pDest, + LONG lDestStride, + const BYTE* pSrc, + LONG lSrcStride, + DWORD dwWidthInPixels, + DWORD dwHeightInPixels +) +{ + if (pDest == NULL || pSrc == NULL) + return; + + for (DWORD y = 0; y < dwHeightInPixels; y++) + { + BGRAPixel* pDestPel = (BGRAPixel*)pDest; + WORD * pSrcPel = (WORD*)pSrc; + + for (DWORD x = 0; x < dwWidthInPixels; x += 2) + { + // Byte order is Y0 U0 Y1 V0 /// NOTE: On the Intel site this comment was wrong. + + int y0 = (int)LOBYTE(pSrcPel[x]); + int u0 = (int)HIBYTE(pSrcPel[x]); + int y1 = (int)LOBYTE(pSrcPel[x + 1]); + int v0 = (int)HIBYTE(pSrcPel[x + 1]); + + pDestPel[x] = ConvertYCrCbToRGB(y0, v0, u0); + pDestPel[x + 1] = ConvertYCrCbToRGB(y1, v0, u0); + } + + pSrc += lSrcStride; + pDest += lDestStride; + } + +} + + + int WINAPI WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine,int nCmdShow) { //debug console @@ -48,7 +113,31 @@ int WINAPI WinMain( HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLin while (msg.message != WM_QUIT) { - RGB24_to_BGRA32(bgraBuffer,m->rawData, m->width, m->height); + if(m->videoFormat == MFVideoFormat_RGB24) + { + RGB24_to_BGRA32(bgraBuffer, m->rawData, m->width, m->height); + } + else if (m->videoFormat == MFVideoFormat_YUY2) + { + // Dest stride in the following call is hardwired to 4 * width because it is RGBA (i.e. 4 bytes per pixel) + TransformImage_YUY2((BYTE*)(bgraBuffer),4* m->width, m->rawData, m->stride, m->width, m->height); + } + else if (m->videoFormat == MFVideoFormat_RGB32) + { + std::cerr << "Unsupported type MFVideoFormat_RGB32" << std::endl; + } + else if (m->videoFormat == MFVideoFormat_NV12) + { + std::cerr << "Unsupported type MFVideoFormat_NV12" << std::endl; + } + else + { + // Houston we have a problem... + /// Note: people are welcome to paste the relevant function here to test + std::cerr << "Unsupported video type" << std::endl; + } + + window.Draw((BYTE*)bgraBuffer, m->width, m->height); while (PeekMessage(&msg, 0, 0, 0, PM_REMOVE)) {