Hi,
Il 16/06/22 14:33, Nikolay Sivov ha scritto:
I don't understand this. Why would it use fractions of strides? And doing one copy instead of two related to odd heights? Or does the subject need adjusting?
Yeah, maybe it's better to explain; first, notice that when the height is even mine code and yours are precisely equivalent: copying x/2 lines of data with stride y and then other x/2 with the same stride y, but offsetted by a y/2, is precisely equivalent to copying x lines with stride y/2.
For example, suppose that width == 4, stride == 8 and lines == 4. You may represent the UV part of the image in this way (where "." denotes padding data):
XX..YY.. XX..YY..
If you use the approach currently in the code, you will first copy all the X data and then all the Y data. But the same data can also be represented as
XX.. YY.. XX.. YY..
I.e., with the double number of lines and half the stride. The code I am proposing "looks" at data in this way and does the copy with a single call to MFCopyImage().
Looking from this point of view, the change is correct, but not particularly useful. The point is that for odd heights the two approaches are different, and from my tests it seems that mine is correct.
Specifically, if width == 4, stride == 8 and lines == 5 the data will look like:
XX..YY.. XX..YY.. XX..
The current code will only copy the first two XX blocks (because it copies lines / 2 == 2 lines), but I think that Microsoft thinks that the third XX line is still part of the image. The new code will correctly copy all the five lines.
Does this make sense?
static inline struct buffer *impl_from_IMFMediaBuffer(IMFMediaBuffer *iface) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 1b5e8cf7ed5..52608c05929 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -6027,12 +6027,11 @@ static void test_MFCreate2DMediaBuffer(void) case MAKEFOURCC('I','M','C','2'): case MAKEFOURCC('I','M','C','4'): - for (j = ptr->height; j < length2 / stride; j++) + for (j = 0; ptr->height * stride + j * (stride / 2) < length2; j++) for (k = 0; k < ptr->width / 2; k++) - ok(data[j * pitch + k] == 0xff, "Unexpected byte %02x at test %d row %d column %d.\n", data[j * pitch + k], i, j, k); - for (j = ptr->height; j < length2 / stride; j++) - for (k = pitch / 2; k < pitch / 2 + ptr->width / 2; k++) - ok(data[j * pitch + k] == 0xff, "Unexpected byte %02x at test %d row %d column %d.\n", data[j * pitch + k], i, j, k); + ok(data[ptr->height * pitch + j * (pitch / 2)
- k] == 0xff,
+ "Unexpected byte %02x at test %d row %d column %d.\n", + data[ptr->height * pitch + j * (pitch / 2) + k], i, j, k); break;
It feels like this could be more readable. I see initial contents are set to 0xff, so I'm not even sure what we are testing. What else could it be if not 0xff?
Yeah, the image format is a bit involved so I have to do some trickery to do the right checks. The point is that what we set of 0xff is the buffer locked with _Lock(), therefore in its contiguous format. When we then lock it with _Lock2D() there will be a lot of padding bytes that are not necessarily set to 0xff (though they could be, I don't think they're guaranteed to be zero). What that test is checking is which bytes of the 2D buffer are really part of the image (i.e., they're not padding), and the result is that the last half-line of the UV section of a image of odd height is still considered part of the image (therefore the change made in the first hunk is appropriate).
You can try to keep the second hunk and revert the first one, and you'll see that the tests fail on Wine (but they don't on Windows).
Thanks, Giovanni.