[PATCH v3 2/8] mfplat/buffer: Fix image copy function for IMC2/IMC4 buffers with odd height.

[Giovanni Mascellani]

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.