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@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);