[PATCH 5/7] qedit: Implement IMediaDet::GetBitmapBits.
Zebediah Figura
zfigura at codeweavers.com
Fri Oct 23 11:05:28 CDT 2020
On 10/21/20 7:57 AM, Gabriel Ivăncescu wrote:
> 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 understand, and sympathize, but at the same time, it's not a good
argument for accepting a patch...
>
> 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)
It can probably be tested in a similar way to the d3d tests [e.g.
compare_color() in a loop.] Failing that, though, just using a solid
color isn't awful.
>
>>>
>>> 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.
A lot of components in the graph *can* be "messed with", yes. I'm not
sure that it's actually worth trying to verify that they're not all the
time, though, because as a rule, applications get to keep the pieces if
they break things.
>
>>> +
>>> + 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);
>>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201023/348cee40/attachment.sig>
More information about the wine-devel
mailing list