[PATCH] mfplat: Properly align memory allocated by IMF2DBuffer.

[Nikolay Sivov]

From: Giovanni Mascellani <gmascellani(a)codeweavers.com>

Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
---

Contains changes from 228247 and 228249.

 dlls/mfplat/buffer.c       |  2 +-
 dlls/mfplat/tests/mfplat.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/dlls/mfplat/buffer.c b/dlls/mfplat/buffer.c
index 1cb84cba944..2ec714c1419 100644
--- a/dlls/mfplat/buffer.c
+++ b/dlls/mfplat/buffer.c
@@ -1385,7 +1385,7 @@ static HRESULT create_2d_buffer(DWORD width, DWORD height, DWORD fourcc, BOOL bo
             max_length = pitch * height;
     }
 
-    if (FAILED(hr = memory_buffer_init(object, max_length, MF_1_BYTE_ALIGNMENT, &memory_1d_2d_buffer_vtbl)))
+    if (FAILED(hr = memory_buffer_init(object, max_length, row_alignment, &memory_1d_2d_buffer_vtbl)))
     {
         free(object);
         return hr;
diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c
index d9c25edaa72..dcb064daf85 100644
--- a/dlls/mfplat/tests/mfplat.c
+++ b/dlls/mfplat/tests/mfplat.c
@@ -6005,6 +6005,31 @@ static void test_MFCreate2DMediaBuffer(void)
 
         IMFMediaBuffer_Release(buffer);
     }
+
+    /* Alignment tests */
+    for (i = 0; i < ARRAY_SIZE(_2d_buffer_tests); ++i)
+    {
+        const struct _2d_buffer_test *ptr = &_2d_buffer_tests[i];
+
+        hr = pMFCreate2DMediaBuffer(ptr->width, ptr->height, ptr->fourcc, FALSE, &buffer);
+        ok(hr == S_OK, "Failed to create a buffer, hr %#lx.\n", hr);
+
+        hr = IMFMediaBuffer_QueryInterface(buffer, &IID_IMF2DBuffer, (void **)&_2dbuffer);
+        ok(hr == S_OK, "Failed to get interface, hr %#lx.\n", hr);
+
+        hr = IMF2DBuffer_Lock2D(_2dbuffer, &data, &pitch);
+        ok(hr == S_OK, "Failed to lock buffer, hr %#lx.\n", hr);
+        ok(((uintptr_t)data & (pitch - 1)) == 0, "Misaligned data at %p.\n", data);
+
+        ok(pitch == ptr->pitch, "%d: unexpected pitch %ld, expected %d, %u x %u, format %s.\n", i, pitch, ptr->pitch,
+                ptr->width, ptr->height, wine_dbgstr_an((char *)&ptr->fourcc, 4));
+
+        hr = IMF2DBuffer_Unlock2D(_2dbuffer);
+        ok(hr == S_OK, "Failed to unlock buffer, hr %#lx.\n", hr);
+
+        IMF2DBuffer_Release(_2dbuffer);
+        IMFMediaBuffer_Release(buffer);
+    }
 }
 
 static void test_MFCreateMediaBufferFromMediaType(void)
-- 
2.35.1

[Giovanni Mascellani]

Hi,

Il 14/03/22 09:40, Nikolay Sivov ha scritto:
> +    /* Alignment tests */
> +    for (i = 0; i < ARRAY_SIZE(_2d_buffer_tests); ++i)
> +    {
> +        const struct _2d_buffer_test *ptr = &_2d_buffer_tests[i];
> +
> +        hr = pMFCreate2DMediaBuffer(ptr->width, ptr->height, ptr->fourcc, FALSE, &buffer);
> +        ok(hr == S_OK, "Failed to create a buffer, hr %#lx.\n", hr);
> +
> +        hr = IMFMediaBuffer_QueryInterface(buffer, &IID_IMF2DBuffer, (void **)&_2dbuffer);
> +        ok(hr == S_OK, "Failed to get interface, hr %#lx.\n", hr);
> +
> +        hr = IMF2DBuffer_Lock2D(_2dbuffer, &data, &pitch);
> +        ok(hr == S_OK, "Failed to lock buffer, hr %#lx.\n", hr);
> +        ok(((uintptr_t)data & (pitch - 1)) == 0, "Misaligned data at %p.\n", data);

This doesn't really make sense to me: pitch is not necessarily a power 
of two. I think that either you just test with 64 bytes alignment (and 
ignore stronger alignments), or you annotate the tests to know which 
require 64 bytes and which require 128 byte.

Thanks, Giovanni.

[Nikolay Sivov]

On 3/14/22 16:59, Giovanni Mascellani wrote:
> Hi,
>
> Il 14/03/22 09:40, Nikolay Sivov ha scritto:
> > +    /* Alignment tests */
> > +    for (i = 0; i < ARRAY_SIZE(_2d_buffer_tests); ++i)
> > +    {
> > +        const struct _2d_buffer_test *ptr = &_2d_buffer_tests[i];
> > +
> > +        hr = pMFCreate2DMediaBuffer(ptr->width, ptr->height, 
> > ptr->fourcc, FALSE, &buffer);
> > +        ok(hr == S_OK, "Failed to create a buffer, hr %#lx.\n", hr);
> > +
> > +        hr = IMFMediaBuffer_QueryInterface(buffer, &IID_IMF2DBuffer, 
> > (void **)&_2dbuffer);
> > +        ok(hr == S_OK, "Failed to get interface, hr %#lx.\n", hr);
> > +
> > +        hr = IMF2DBuffer_Lock2D(_2dbuffer, &data, &pitch);
> > +        ok(hr == S_OK, "Failed to lock buffer, hr %#lx.\n", hr);
> > +        ok(((uintptr_t)data & (pitch - 1)) == 0, "Misaligned data at 
> > %p.\n", data);
>
> This doesn't really make sense to me: pitch is not necessarily a power 
> of two. I think that either you just test with 64 bytes alignment (and 
> ignore stronger alignments), or you annotate the tests to know which 
> require 64 bytes and which require 128 byte.

All existing tests are already either 64 or 128.
> Thanks, Giovanni.

[Giovanni Mascellani]

Il 14/03/22 15:03, Nikolay Sivov ha scritto:
> > This doesn't really make sense to me: pitch is not necessarily a power 
> > of two. I think that either you just test with 64 bytes alignment (and 
> > ignore stronger alignments), or you annotate the tests to know which 
> > require 64 bytes and which require 128 byte.
>
> All existing tests are already either 64 or 128.

This means that the tests will break as soon as somebody adds larger 
widths for any reason. IMHO a more robust logic would serve us better.

Giovanni.