[PATCH 5/7] qedit: Implement IMediaDet::GetBitmapBits.
Gabriel Ivăncescu
gabrielopcode at gmail.com
Wed Oct 21 07:57:24 CDT 2020
On 20/10/2020 20:27, Zebediah Figura wrote:
> On 10/19/20 11:48 AM, Gabriel Ivăncescu wrote:
>> The stretching algorithm that Windows uses does not match any mode on
>> StretchBlt or StretchDIBits, which is why the actual algorithm used by
>> these APIs was implemented.
>>
>> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
>> ---
>>
>> I've literally tested every mode (on SetStretchBltMode) and operation mode
>> and none of them were correct on Windows, even if Wine's implementation is
>> not perfect. So clearly Windows doesn't use gdi32 to do the scaling here. It
>> was quite a pain to discover the exact algorithm used, but this passes all
>> tests on all Windows versions on the testbot.
>
> I appreciate the attention to detail, and the effort put into
> reverse-engineering this algorithm. However, pixel-perfect accuracy is
> not really a goal of Wine, especially not when we're relying on
> libraries like GStreamer to do decoding and format conversions, and in
> this case I don't think that it's worth writing or maintaining a new
> stretching algorithm. I would instead just use StretchDIBits() with the
> closest stretch blit mode.
>
Hmm, can we please keep it this time? I have several reasons of course,
but I'd also like to avoid feel like I wasted all that effort for
nothing. :-)
I think there might be a reason Microsoft used a different stretching
than gdi32, and since I've already done it... it's not that complicated
either, it's quite a simple algorithm.
More importantly, if we don't have an exact stretch, we won't be able to
test it at all, not even on Windows. It's not a question of todo_wine,
we won't be able to test it *at all*.
I'd have to remove all the stretching tests on the bitmap data. This was
actually my main motivator to reverse engineer it, after all. Are you
sure that's a good idea, seeing as I already uncovered it now? (so
there's no extra effort involved, because it passes on *all* Windows
versions, from XP to the latest Windows 10)
>>
>> dlls/qedit/mediadet.c | 191 +++++++++++++++++++++++++++++++++++-
>> dlls/qedit/tests/mediadet.c | 57 ++++++-----
>> 2 files changed, 222 insertions(+), 26 deletions(-)
>>
>> diff --git a/dlls/qedit/mediadet.c b/dlls/qedit/mediadet.c
>> index 6afae37..f874095 100644
>> --- a/dlls/qedit/mediadet.c
>> +++ b/dlls/qedit/mediadet.c
>> @@ -324,6 +324,60 @@ done:
>> return hr;
>> }
>>
>> +static void stretch_line(BYTE *dst, ULONG dst_width, BYTE *src, ULONG src_width)
>> +{
>> + ULONG ratio, rem, drift, i = dst_width;
>> +
>> + if (dst_width < src_width)
>> + {
>> + ratio = src_width / dst_width;
>> + rem = src_width % dst_width;
>> + drift = 0;
>> + while (i--)
>> + {
>> + dst[0] = src[0];
>> + dst[1] = src[1];
>> + dst[2] = src[2];
>> + dst += 3;
>> + src += ratio * 3;
>> + if (drift < rem)
>> + {
>> + src += 3;
>> + drift += dst_width;
>> + }
>> + drift -= rem;
>> + }
>> + }
>> + else if (dst_width > src_width)
>> + {
>> + drift = dst_width - 1;
>> + while (i--)
>> + {
>> + dst[0] = src[0];
>> + dst[1] = src[1];
>> + dst[2] = src[2];
>> + dst += 3;
>> + if (drift < src_width)
>> + {
>> + drift += dst_width;
>> + src += 3;
>> + }
>> + drift -= src_width;
>> + }
>> + }
>> + else
>> + {
>> + memcpy(dst, src, dst_width * 3);
>> + dst += dst_width * 3;
>> + }
>> +
>> + /* Fill the stride padding with zeros */
>> + i = (dst_width * 3) % 4;
>> + if (i)
>> + for (i = 4 - i; i--;)
>> + *dst++ = 0;
>> +}
>> +
>> /* MediaDet inner IUnknown */
>> static HRESULT WINAPI MediaDet_inner_QueryInterface(IUnknown *iface, REFIID riid, void **ppv)
>> {
>> @@ -727,10 +781,139 @@ static HRESULT WINAPI MediaDet_GetBitmapBits(IMediaDet* iface,
>> LONG *pBufferSize, char *pBuffer,
>> LONG Width, LONG Height)
>> {
>> - MediaDetImpl *This = impl_from_IMediaDet(iface);
>> - FIXME("(%p)->(%f %p %p %d %d): not implemented!\n", This, StreamTime, pBufferSize, pBuffer,
>> - Width, Height);
>> - return E_NOTIMPL;
>> + MediaDetImpl *detector = impl_from_IMediaDet(iface);
>> + LONG ratio, rem, stride = (Width * 3 + 3) & ~3;
>> + LONG src_x, src_y, src_stride, size, buf_size;
>> + ULONG src_width, src_height, drift, i;
>> + BITMAPINFOHEADER hdr = { 0 };
>> + BYTE *buf, *dup, *dst, *src;
>> + VIDEOINFOHEADER *info;
>> + AM_MEDIA_TYPE mt;
>> + HRESULT hr;
>> +
>> + TRACE("(%p)->(%f %p %p %d %d)\n", detector, StreamTime, pBufferSize, pBuffer, Width, Height);
>> +
>> + if (!pBuffer && !pBufferSize) return E_POINTER;
>> + if (Width < 0 || Height < 0) return E_INVALIDARG;
>> + if (!pBuffer)
>> + {
>> + *pBufferSize = sizeof(BITMAPINFOHEADER) + stride * Height;
>> + return S_OK;
>> + }
>> + if (StreamTime < 0.0) return E_INVALIDARG;
>> + if (pBufferSize)
>> + {
>> + if (*pBufferSize < 0)
>> + return E_INVALIDARG;
>> + if (*pBufferSize < sizeof(BITMAPINFOHEADER) + stride * Height)
>> + return E_OUTOFMEMORY;
>> + }
>> +
>> + if (detector->grabber)
>> + hr = seek_source(detector, StreamTime);
>> + else
>> + hr = IMediaDet_EnterBitmapGrabMode(&detector->IMediaDet_iface, StreamTime);
>> + if (FAILED(hr))
>> + return hr;
>> +
>> + hr = ISampleGrabber_GetConnectedMediaType(detector->grabber, &mt);
>> + info = (VIDEOINFOHEADER*)mt.pbFormat;
>> + if (FAILED(hr) ||
>> + !IsEqualGUID(&mt.majortype, &MEDIATYPE_Video) ||
>> + !IsEqualGUID(&mt.subtype, &MEDIASUBTYPE_RGB24) ||
>> + !IsEqualGUID(&mt.formattype, &FORMAT_VideoInfo) ||
>> + mt.cbFormat != sizeof(VIDEOINFOHEADER) ||
>> + info->bmiHeader.biSize != sizeof(BITMAPINFOHEADER) ||
>> + info->bmiHeader.biWidth <= 0 ||
>> + info->bmiHeader.biHeight == 0 ||
>> + info->bmiHeader.biPlanes != 1 ||
>> + info->bmiHeader.biBitCount != 24 ||
>> + info->bmiHeader.biCompression != BI_RGB)
>> + return VFW_E_INVALID_MEDIA_TYPE;
>
> This may leak "mt.pbFormat". Moreover, can some of these conditions even
> happen? And are the others worth checking for?
>
Oops you're right. And I check them rigorously just to be sure, because
the Sample Grabber can be messed with, I believe.
>> +
>> + src_x = src_y = 0;
>> + src_width = info->bmiHeader.biWidth;
>> + src_height = abs(info->bmiHeader.biHeight);
>> + src_stride = (src_width * 3 + 3) & ~3;
>> + buf_size = src_stride * src_height;
>> + if (!IsRectEmpty(&info->rcTarget))
>> + {
>> + src_x = max(info->rcTarget.left, 0);
>> + src_y = max(info->rcTarget.top, 0);
>> + src_width = min(info->rcTarget.right - src_x, src_width);
>> + src_height = min(info->rcTarget.bottom - src_y, src_height);
>> + }
>> + if (info->bmiHeader.biHeight < 0)
>> + {
>> + src_y += info->bmiHeader.biHeight + 1;
>> + src_stride = -src_stride;
>> + }
>> + FreeMediaType(&mt);
>> +
>> + if (!(buf = HeapAlloc(GetProcessHeap(), 0, buf_size)))
>> + return E_OUTOFMEMORY;
>> + size = buf_size;
>> + hr = ISampleGrabber_GetCurrentBuffer(detector->grabber, &size, (LONG*)buf);
>> + if (FAILED(hr)) goto err;
>> + if (size != buf_size)
>> + {
>> + hr = E_UNEXPECTED;
>> + goto err;
>> + }
>> +
>> + hdr.biSize = sizeof(BITMAPINFOHEADER);
>> + hdr.biWidth = Width;
>> + hdr.biHeight = Height;
>> + hdr.biPlanes = 1;
>> + hdr.biBitCount = 24;
>> + hdr.biCompression = BI_RGB;
>> + memcpy(pBuffer, &hdr, sizeof(BITMAPINFOHEADER));
>> +
>> + /* Copy and potentially stretch the image (differently than StretchBlt) */
>> + dst = (BYTE*)pBuffer + sizeof(BITMAPINFOHEADER);
>> + src = buf + src_x * 3 + src_y * src_stride;
>> + i = Height;
>> + if (Height < src_height)
>> + {
>> + ratio = src_height / Height;
>> + rem = src_height % Height;
>> + drift = 0;
>> + while (i--)
>> + {
>> + stretch_line(dst, Width, src, src_width);
>> + dst += stride;
>> + src += ratio * src_stride;
>> + if (drift < rem)
>> + {
>> + src += src_stride;
>> + drift += Height;
>> + }
>> + drift -= rem;
>> + }
>> + }
>> + else
>> + {
>> + drift = Height - 1;
>> + while (i--)
>> + {
>> + stretch_line(dst, Width, src, src_width);
>> + dup = dst;
>> + dst += stride;
>> + while (drift >= src_height && i--)
>> + {
>> + memcpy(dst, dup, stride);
>> + dst += stride;
>> + drift -= src_height;
>> + }
>> + drift += Height - src_height;
>> + src += src_stride;
>> + }
>> + }
>> +
>> + hr = S_OK;
>> +err:
>> + HeapFree(GetProcessHeap(), 0, buf);
>> + return hr;
>> }
>>
>> static HRESULT WINAPI MediaDet_WriteBitmapBits(IMediaDet* iface,
>> diff --git a/dlls/qedit/tests/mediadet.c b/dlls/qedit/tests/mediadet.c
>> index 98372b6..e00226a 100644
>> --- a/dlls/qedit/tests/mediadet.c
>> +++ b/dlls/qedit/tests/mediadet.c
>> @@ -1531,10 +1531,23 @@ static void test_bitmap_grab_mode(void)
>> ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>> size = sizeof(BITMAPINFOHEADER) + 640 * 480 * 3;
>> hr = IMediaDet_GetBitmapBits(detector, 0.0, &size, buf, 640, 480);
>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>> hr = IMediaDet_GetSampleGrabber(detector, &sg);
>> ok(hr == E_NOINTERFACE, "Got hr %#x.\n", hr);
>>
>> + /* Lines are rounded up to the bitmap stride */
>> + hr = IMediaDet_GetBitmapBits(detector, 0.0, &size, NULL, 640, 480);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ok(size == sizeof(BITMAPINFOHEADER) + 640 * 480 * 3, "Got size %d.\n", size);
>> + hr = IMediaDet_GetBitmapBits(detector, 0.0, &size, NULL, 640, 0);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ok(size == sizeof(BITMAPINFOHEADER), "Got size %d.\n", size);
>> + hr = IMediaDet_GetBitmapBits(detector, -1.0, &size, NULL, 151, 131);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ok(size == sizeof(BITMAPINFOHEADER) + ((151 * 3 + 3) & ~3) * 131, "Got size %d.\n", size);
>> + hr = IMediaDet_GetBitmapBits(detector, 0.0, &size, NULL, -59, -79);
>> + ok(hr == E_INVALIDARG || broken(hr == S_OK) /* WinXP */, "Got hr %#x.\n", hr);
>> +
>> /* EnterBitmapGrabMode only seeks once, and if SeekTime is non-negative */
>> testfilter_init(&testfilter);
>> testfilter.bitmap_grab_mode = TRUE;
>> @@ -1681,39 +1694,39 @@ static void test_bitmap_grab_mode(void)
>>
>> /* despite what MSDN states, size must be properly supplied on newer Windows versions */
>> hr = IMediaDet_GetBitmapBits(detector, 0.0, NULL, NULL, 640, 480);
>> - todo_wine ok(hr == E_POINTER, "Got hr %#x.\n", hr);
>> + ok(hr == E_POINTER, "Got hr %#x.\n", hr);
>> size = -1;
>> hr = IMediaDet_GetBitmapBits(detector, 0.0, &size, buf, 640, 480);
>> - todo_wine ok(hr == E_INVALIDARG || broken(hr == S_OK) /* WinXP */, "Got hr %#x.\n", hr);
>> + ok(hr == E_INVALIDARG || broken(hr == S_OK) /* WinXP */, "Got hr %#x.\n", hr);
>> size = 640 * 480 * 3;
>> hr = IMediaDet_GetBitmapBits(detector, 0.0, &size, buf, 640, 480);
>> - todo_wine ok(hr == E_OUTOFMEMORY || broken(hr == S_OK) /* WinXP */, "Got hr %#x.\n", hr);
>> + ok(hr == E_OUTOFMEMORY || broken(hr == S_OK) /* WinXP */, "Got hr %#x.\n", hr);
>> ok(size == 640 * 480 * 3, "Got size %d.\n", size);
>> size = sizeof(BITMAPINFOHEADER) + 640 * 480 * 3;
>> hr = IMediaDet_GetBitmapBits(detector, -1.0, &size, buf, 640, 480);
>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>> hr = IMediaDet_GetBitmapBits(detector, 2.5, &size, buf, 640, 480);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> - todo_wine check_bitmap(buf, 640, 480, 2.5);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + check_bitmap(buf, 640, 480, 2.5);
>>
>> /* GetBitmapBits/WriteBitmapBits can scale the image */
>> size = sizeof(BITMAPINFOHEADER) + 960 * 720 * 3;
>> hr = IMediaDet_GetBitmapBits(detector, 1.5, &size, buf, 131, 151);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> ok(size == sizeof(BITMAPINFOHEADER) + 960 * 720 * 3, "Got size %d.\n", size);
>> - todo_wine check_bitmap(buf, 131, 151, 1.5);
>> + check_bitmap(buf, 131, 151, 1.5);
>> hr = IMediaDet_GetBitmapBits(detector, 4.0, NULL, buf, 503, 79);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> - todo_wine check_bitmap(buf, 503, 79, 4.0);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + check_bitmap(buf, 503, 79, 4.0);
>> hr = IMediaDet_GetBitmapBits(detector, 1.52, NULL, buf, 139, 487);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> - todo_wine check_bitmap(buf, 139, 487, 1.52);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + check_bitmap(buf, 139, 487, 1.52);
>> hr = IMediaDet_GetBitmapBits(detector, 2.12, NULL, buf, 640, 641);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> - todo_wine check_bitmap(buf, 640, 641, 2.12);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + check_bitmap(buf, 640, 641, 2.12);
>> hr = IMediaDet_GetBitmapBits(detector, 3.25, NULL, buf, 960, 720);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> - todo_wine check_bitmap(buf, 960, 720, 3.25);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + check_bitmap(buf, 960, 720, 3.25);
>>
>> /* Changing filter resets bitmap grab mode */
>> testfilter.bitmap_grab_mode = FALSE;
>> @@ -1728,7 +1741,7 @@ static void test_bitmap_grab_mode(void)
>> /* GetBitmapBits enables it only if it retrieves the image */
>> testfilter.bitmap_grab_mode = TRUE;
>> hr = IMediaDet_GetBitmapBits(detector, 1.25, &size, NULL, 640, 480);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> hr = IMediaDet_GetSampleGrabber(detector, &sg);
>> ok(hr == E_NOINTERFACE, "Got hr %#x.\n", hr);
>> hr = IMediaDet_get_OutputStreams(detector, &count);
>> @@ -1736,12 +1749,12 @@ static void test_bitmap_grab_mode(void)
>> ok(count == 1, "Got %d streams.\n", count);
>>
>> hr = IMediaDet_GetBitmapBits(detector, 1.25, NULL, buf, 640, 480);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> hr = IMediaDet_GetSampleGrabber(detector, &sg);
>> - todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
>> - if (SUCCEEDED(hr)) ISampleGrabber_Release(sg);
>> + ok(hr == S_OK, "Got hr %#x.\n", hr);
>> + ISampleGrabber_Release(sg);
>> hr = IMediaDet_get_OutputStreams(detector, &count);
>> - todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>> + ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
>>
>> ref = IMediaDet_Release(detector);
>> ok(!ref, "Got outstanding refcount %d.\n", ref);
>>
>
More information about the wine-devel
mailing list