Much of this patch doesn't make sense without 4/5. Could the two patches be squashed together?
On 2/22/22 16:48, Rémi Bernon wrote:
- if (FAILED(hr = IMFSample_GetBufferCount(sample, &count)) || count != 1)
- {
FIXME("Only samples with 1 buffer are supported!\n");return hr ? hr : E_FAIL;- }
This made me do a double take. Since these conditions don't really share their bodies, perhaps they should be separated?
+void mf_destroy_wg_sample(struct wg_sample *sample) +{
- IMFMediaBuffer *media_buffer;
- HRESULT hr;
- if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer)))
WARN("Failed to get first buffer, sample %p\n", sample->private);- else
- {
/* release lock and ref taken in mf_create_wg_sample */IMFMediaBuffer_Unlock(media_buffer);IMFMediaBuffer_SetCurrentLength(media_buffer, sample->size);IMFMediaBuffer_Release(media_buffer);
Why are we holding a reference to the media buffer? (And if we need to, should we be storing that instead of the IMFSample?)
IMFMediaBuffer_Release(media_buffer);- }
- IMFSample_Release(sample->private);
- free(sample);
+} diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 4adbb694766..59b15df7d08 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -103,6 +103,14 @@ struct wg_format } u; };
+struct wg_sample +{
- UINT32 max_size;
- UINT32 size;
- BYTE *data;
- void *private;
+};
"max_size" isn't relevant until patch 5/5.
"private" also doesn't particularly seem like it belongs here. In particular, if you're going to wrap the wg_sample in another structure in patch 5/5, I'd rather see the same approach taken for frontend-specific data.
- if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer)))
return hr;- IMFMediaBuffer_Release(media_buffer);
This seems pointless (and hence, at least, doesn't belong in this patch). Or does ConvertToContiguousBuffer() also modify the original sample?
- if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize))
- {
mf_destroy_wg_sample(wg_sample);return S_OK;- }
I can't tell if this is trying to align to info.cbSize or (as the documentation specifies) treat it as a minimum, but either way I think it's broken.
- decoder->input_sample = wg_sample;
Do we have to store this for API compatibility? If so it would seem helpful to specify that in a comment. It looks wrong as-is, since we don't need the data after we copy it.
(And depending on how hard we need to preserve API compatibility in this area, I'm concerned that patch 5/5 may be making assumptions about how GStreamer uses buffers that aren't actually guaranteed.)