[PATCH 0/5] MR242: winegstreamer: Don't try using src_getrange_cb input buffer.
From: Rémi Bernon <rbernon(a)codeweavers.com> It is invalid according to GStreamer documentation. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 7d55897aa0a..ca95f187aea 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -167,8 +167,7 @@ static NTSTATUS wg_parser_push_data(void *args) /* Note that we don't allocate the buffer until we have a size. * midiparse passes a NULL buffer and a size of UINT_MAX, in an * apparent attempt to read the whole input stream at once. */ - if (!parser->read_request.buffer) - parser->read_request.buffer = gst_buffer_new_and_alloc(size); + parser->read_request.buffer = gst_buffer_new_and_alloc(size); gst_buffer_map(parser->read_request.buffer, &map_info, GST_MAP_WRITE); memcpy(map_info.data, data, size); gst_buffer_unmap(parser->read_request.buffer, &map_info); @@ -886,9 +885,7 @@ static GstFlowReturn src_getrange_cb(GstPad *pad, GstObject *parent, /* asfreader occasionally asks for zero bytes. gst_buffer_map() will * return NULL in this case. Avoid confusing the read thread by asking * it for zero bytes. */ - if (!*buffer) - *buffer = gst_buffer_new_and_alloc(0); - gst_buffer_set_size(*buffer, 0); + *buffer = gst_buffer_new_and_alloc(0); GST_LOG("Returning empty buffer."); return GST_FLOW_OK; } @@ -896,7 +893,7 @@ static GstFlowReturn src_getrange_cb(GstPad *pad, GstObject *parent, pthread_mutex_lock(&parser->mutex); assert(!parser->read_request.size); - parser->read_request.buffer = *buffer; + parser->read_request.buffer = NULL; parser->read_request.offset = offset; parser->read_request.size = size; parser->read_request.done = false; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/242
On 6/14/22 02:26, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
It is invalid according to GStreamer documentation.
What documentation are you referring to? The documentation for GstPadGetRangeFunction itself is somewhat badly written and doesn't actually mention what "buffer" is expected to contain on input, but the documentation for gst_pad_get_range() says: If buffer points to a variable holding NULL, a valid new GstBuffer will be placed in buffer when this function returns GST_FLOW_OK. The new buffer must be freed with gst_buffer_unref after usage. When buffer points to a variable that points to a valid GstBuffer, the buffer will be filled with the result data when this function returns GST_FLOW_OK. If the provided buffer is larger than size, only size bytes will be filled in the result buffer and its size will be updated accordingly. and the implementation shows that the intent is for the same to apply to GstPadGetRangeFunction. For what it's worth, I believe oggdemux actually does pass a non-NULL buffer, and breaks if we don't fill that.
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_parser.c | 41 ++++++++++++++-------------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index ca95f187aea..865b6d3643a 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -155,33 +155,26 @@ static NTSTATUS wg_parser_push_data(void *args) struct wg_parser *parser = params->parser; const void *data = params->data; uint32_t size = params->size; - - pthread_mutex_lock(&parser->mutex); - - if (data) - { - if (size) - { - GstMapInfo map_info; - - /* Note that we don't allocate the buffer until we have a size. - * midiparse passes a NULL buffer and a size of UINT_MAX, in an - * apparent attempt to read the whole input stream at once. */ - parser->read_request.buffer = gst_buffer_new_and_alloc(size); - gst_buffer_map(parser->read_request.buffer, &map_info, GST_MAP_WRITE); - memcpy(map_info.data, data, size); - gst_buffer_unmap(parser->read_request.buffer, &map_info); - parser->read_request.ret = GST_FLOW_OK; - } - else - { - parser->read_request.ret = GST_FLOW_EOS; - } - } + GstBuffer *buffer = NULL; + GstFlowReturn result; + + if (!data) + result = GST_FLOW_ERROR; + else if (!size) + result = GST_FLOW_EOS; + else if (!(buffer = gst_buffer_new_and_alloc(size))) + result = GST_FLOW_ERROR; else { - parser->read_request.ret = GST_FLOW_ERROR; + gst_buffer_fill(buffer, 0, data, size); + GST_INFO("Copied %u bytes from data %p to buffer %p", size, data, buffer); + result = GST_FLOW_OK; } + + pthread_mutex_lock(&parser->mutex); + + parser->read_request.ret = result; + parser->read_request.buffer = buffer; parser->read_request.done = true; parser->read_request.size = 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/242
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/media_source.c | 75 ++++++++++++--------- dlls/winegstreamer/quartz_parser.c | 49 +++++++++----- dlls/winegstreamer/wm_reader.c | 102 +++++++++++++++-------------- 3 files changed, 129 insertions(+), 97 deletions(-) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index f07b83f413e..ac23b1bc94c 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -592,12 +592,54 @@ static const IMFAsyncCallbackVtbl source_async_commands_callback_vtbl = source_async_commands_Invoke, }; +static void handle_read_request(struct media_source *source, QWORD file_size, + void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) +{ + IMFByteStream *byte_stream = source->byte_stream; + ULONG ret_size = 0; + HRESULT hr; + void *data; + + if (offset >= file_size) + size = 0; + else if (offset + size >= file_size) + size = file_size - offset; + + if (!array_reserve(buffer, buffer_size, size, 1)) + { + wg_parser_push_data(source->wg_parser, NULL, 0); + return; + } + data = *buffer; + + /* Some IMFByteStreams (including the standard file-based stream) return + * an error when reading past the file size. */ + + if (!size) + hr = S_OK; + else if (SUCCEEDED(hr = IMFByteStream_SetCurrentPosition(byte_stream, offset))) + hr = IMFByteStream_Read(byte_stream, data, size, &ret_size); + + if (FAILED(hr)) + { + ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); + data = NULL; + } + else if (ret_size != size) + { + ERR("Unexpected short read: requested %u bytes, got %lu.\n", size, ret_size); + size = ret_size; + } + + wg_parser_push_data(source->wg_parser, data, size); +} + static DWORD CALLBACK read_thread(void *arg) { struct media_source *source = arg; IMFByteStream *byte_stream = source->byte_stream; size_t buffer_size = 4096; - uint64_t file_size; + QWORD file_size; void *data; if (!(data = malloc(buffer_size))) @@ -610,41 +652,12 @@ static DWORD CALLBACK read_thread(void *arg) while (!source->read_thread_shutdown) { uint64_t offset; - ULONG ret_size; uint32_t size; - HRESULT hr; if (!wg_parser_get_next_read_offset(source->wg_parser, &offset, &size)) continue; - if (offset >= file_size) - size = 0; - else if (offset + size >= file_size) - size = file_size - offset; - - /* Some IMFByteStreams (including the standard file-based stream) return - * an error when reading past the file size. */ - if (!size) - { - wg_parser_push_data(source->wg_parser, data, 0); - continue; - } - - if (!array_reserve(&data, &buffer_size, size, 1)) - { - free(data); - return 0; - } - - ret_size = 0; - - if (SUCCEEDED(hr = IMFByteStream_SetCurrentPosition(byte_stream, offset))) - hr = IMFByteStream_Read(byte_stream, data, size, &ret_size); - if (FAILED(hr)) - ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); - else if (ret_size != size) - ERR("Unexpected short read: requested %u bytes, got %lu.\n", size, ret_size); - wg_parser_push_data(source->wg_parser, SUCCEEDED(hr) ? data : NULL, ret_size); + handle_read_request(source, file_size, &data, &buffer_size, offset, size); } free(data); diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 34848c0b503..0e9f183d499 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -865,12 +865,41 @@ static DWORD CALLBACK stream_thread(void *arg) return 0; } +static void handle_read_request(struct parser *filter, LONGLONG file_size, + void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) +{ + HRESULT hr; + void *data; + + if (offset >= file_size) + size = 0; + else if (offset + size >= file_size) + size = file_size - offset; + + if (!array_reserve(buffer, buffer_size, size, 1)) + { + wg_parser_push_data(filter->wg_parser, NULL, 0); + return; + } + data = *buffer; + + hr = IAsyncReader_SyncRead(filter->reader, offset, size, data); + + if (FAILED(hr)) + { + ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); + data = NULL; + } + + wg_parser_push_data(filter->wg_parser, data, size); +} + static DWORD CALLBACK read_thread(void *arg) { struct parser *filter = arg; LONGLONG file_size, unused; size_t buffer_size = 4096; - void *data = NULL; + void *data; if (!(data = malloc(buffer_size))) return 0; @@ -883,27 +912,11 @@ static DWORD CALLBACK read_thread(void *arg) { uint64_t offset; uint32_t size; - HRESULT hr; if (!wg_parser_get_next_read_offset(filter->wg_parser, &offset, &size)) continue; - if (offset >= file_size) - size = 0; - else if (offset + size >= file_size) - size = file_size - offset; - - if (!array_reserve(&data, &buffer_size, size, 1)) - { - free(data); - return 0; - } - - hr = IAsyncReader_SyncRead(filter->reader, offset, size, data); - if (FAILED(hr)) - ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); - - wg_parser_push_data(filter->wg_parser, SUCCEEDED(hr) ? data : NULL, size); + handle_read_request(filter, file_size, &data, &buffer_size, offset, size); } free(data); diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 03adea8a318..9d1eb2a95ed 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -526,6 +526,59 @@ static const IWMMediaPropsVtbl stream_props_vtbl = stream_props_SetMediaType, }; +static void handle_read_request(struct wm_reader *reader, uint64_t file_size, + void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) +{ + IStream *stream = reader->source_stream; + LARGE_INTEGER large_offset; + HANDLE file = reader->file; + ULONG ret_size = 0; + HRESULT hr; + void *data; + + if (offset >= file_size) + size = 0; + else if (offset + size >= file_size) + size = file_size - offset; + + if (!array_reserve(buffer, buffer_size, size, 1)) + { + wg_parser_push_data(reader->wg_parser, NULL, 0); + return; + } + data = *buffer; + + large_offset.QuadPart = offset; + if (!size) + hr = S_OK; + else if (file) + { + if (!SetFilePointerEx(file, large_offset, NULL, FILE_BEGIN) + || !ReadFile(file, data, size, &ret_size, NULL)) + hr = HRESULT_FROM_WIN32(GetLastError()); + else + hr = S_OK; + } + else + { + if (SUCCEEDED(hr = IStream_Seek(stream, large_offset, STREAM_SEEK_SET, NULL))) + hr = IStream_Read(stream, data, size, &ret_size); + } + + if (FAILED(hr)) + { + ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); + data = NULL; + } + else if (ret_size != size) + { + ERR("Unexpected short read: requested %u bytes, got %lu.\n", size, ret_size); + size = ret_size; + } + + wg_parser_push_data(reader->wg_parser, data, size); +} + static DWORD CALLBACK read_thread(void *arg) { struct wm_reader *reader = arg; @@ -557,60 +610,13 @@ static DWORD CALLBACK read_thread(void *arg) while (!reader->read_thread_shutdown) { - LARGE_INTEGER large_offset; uint64_t offset; - ULONG ret_size; uint32_t size; - HRESULT hr; if (!wg_parser_get_next_read_offset(reader->wg_parser, &offset, &size)) continue; - if (offset >= file_size) - size = 0; - else if (offset + size >= file_size) - size = file_size - offset; - - if (!size) - { - wg_parser_push_data(reader->wg_parser, data, 0); - continue; - } - - if (!array_reserve(&data, &buffer_size, size, 1)) - { - free(data); - return 0; - } - - ret_size = 0; - - large_offset.QuadPart = offset; - if (file) - { - if (!SetFilePointerEx(file, large_offset, NULL, FILE_BEGIN) - || !ReadFile(file, data, size, &ret_size, NULL)) - { - ERR("Failed to read %u bytes at offset %I64u, error %lu.\n", size, offset, GetLastError()); - wg_parser_push_data(reader->wg_parser, NULL, 0); - continue; - } - } - else - { - if (SUCCEEDED(hr = IStream_Seek(stream, large_offset, STREAM_SEEK_SET, NULL))) - hr = IStream_Read(stream, data, size, &ret_size); - if (FAILED(hr)) - { - ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); - wg_parser_push_data(reader->wg_parser, NULL, 0); - continue; - } - } - - if (ret_size != size) - ERR("Unexpected short read: requested %u bytes, got %lu.\n", size, ret_size); - wg_parser_push_data(reader->wg_parser, data, ret_size); + handle_read_request(reader, file_size, &data, &buffer_size, offset, size); } free(data); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/242
From: Rémi Bernon <rbernon(a)codeweavers.com> And wg_parser_push_data to wg_parser_reply_read. Wrapping the request in a more general structure so we can later emit buffer pool allocation request to the reader thread. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 4 ++-- dlls/winegstreamer/main.c | 17 ++++++++-------- dlls/winegstreamer/media_source.c | 24 ++++++++++++++++------- dlls/winegstreamer/quartz_parser.c | 24 ++++++++++++++++------- dlls/winegstreamer/unixlib.h | 29 ++++++++++++++++++++++------ dlls/winegstreamer/wg_parser.c | 31 +++++++++++++++--------------- dlls/winegstreamer/wm_reader.c | 24 ++++++++++++++++------- 7 files changed, 100 insertions(+), 53 deletions(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index e1abe7018da..2552e1d7f61 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -76,8 +76,8 @@ void wg_parser_destroy(struct wg_parser *parser); HRESULT wg_parser_connect(struct wg_parser *parser, uint64_t file_size); void wg_parser_disconnect(struct wg_parser *parser); -bool wg_parser_get_next_read_offset(struct wg_parser *parser, uint64_t *offset, uint32_t *size); -void wg_parser_push_data(struct wg_parser *parser, const void *data, uint32_t size); +bool wg_parser_wait_request(struct wg_parser *parser, struct wg_request *request); +void wg_parser_reply_read(struct wg_parser *parser, const void *data, uint32_t size); uint32_t wg_parser_get_stream_count(struct wg_parser *parser); struct wg_parser_stream *wg_parser_get_stream(struct wg_parser *parser, uint32_t index); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 5075b3118cd..25f0c38b700 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -107,25 +107,24 @@ void wg_parser_disconnect(struct wg_parser *parser) __wine_unix_call(unix_handle, unix_wg_parser_disconnect, parser); } -bool wg_parser_get_next_read_offset(struct wg_parser *parser, uint64_t *offset, uint32_t *size) +bool wg_parser_wait_request(struct wg_parser *parser, struct wg_request *wg_request) { - struct wg_parser_get_next_read_offset_params params = + struct wg_parser_wait_request_params params = { .parser = parser, }; - TRACE("parser %p, offset %p, size %p.\n", parser, offset, size); + TRACE("parser %p, wg_request %p.\n", parser, wg_request); - if (__wine_unix_call(unix_handle, unix_wg_parser_get_next_read_offset, ¶ms)) + if (__wine_unix_call(unix_handle, unix_wg_parser_wait_request, ¶ms)) return false; - *offset = params.offset; - *size = params.size; + *wg_request = params.request; return true; } -void wg_parser_push_data(struct wg_parser *parser, const void *data, uint32_t size) +void wg_parser_reply_read(struct wg_parser *parser, const void *data, uint32_t size) { - struct wg_parser_push_data_params params = + struct wg_parser_reply_read_params params = { .parser = parser, .data = data, @@ -134,7 +133,7 @@ void wg_parser_push_data(struct wg_parser *parser, const void *data, uint32_t si TRACE("parser %p, data %p, size %u.\n", parser, data, size); - __wine_unix_call(unix_handle, unix_wg_parser_push_data, ¶ms); + __wine_unix_call(unix_handle, unix_wg_parser_reply_read, ¶ms); } uint32_t wg_parser_get_stream_count(struct wg_parser *parser) diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index ac23b1bc94c..5b98c90a8b4 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -593,9 +593,11 @@ static const IMFAsyncCallbackVtbl source_async_commands_callback_vtbl = }; static void handle_read_request(struct media_source *source, QWORD file_size, - void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) + void **buffer, size_t *buffer_size, struct wg_request *request) { IMFByteStream *byte_stream = source->byte_stream; + uint64_t offset = request->u.read.offset; + uint32_t size = request->u.read.size; ULONG ret_size = 0; HRESULT hr; void *data; @@ -607,7 +609,7 @@ static void handle_read_request(struct media_source *source, QWORD file_size, if (!array_reserve(buffer, buffer_size, size, 1)) { - wg_parser_push_data(source->wg_parser, NULL, 0); + wg_parser_reply_read(source->wg_parser, NULL, 0); return; } data = *buffer; @@ -631,7 +633,7 @@ static void handle_read_request(struct media_source *source, QWORD file_size, size = ret_size; } - wg_parser_push_data(source->wg_parser, data, size); + wg_parser_reply_read(source->wg_parser, data, size); } static DWORD CALLBACK read_thread(void *arg) @@ -651,13 +653,21 @@ static DWORD CALLBACK read_thread(void *arg) while (!source->read_thread_shutdown) { - uint64_t offset; - uint32_t size; + struct wg_request request; - if (!wg_parser_get_next_read_offset(source->wg_parser, &offset, &size)) + if (!wg_parser_wait_request(source->wg_parser, &request)) continue; - handle_read_request(source, file_size, &data, &buffer_size, offset, size); + switch (request.type) + { + case WG_REQUEST_READ: + handle_read_request(source, file_size, &data, &buffer_size, &request); + break; + + default: + FIXME("Ingoring unknown request %u\n", request.type); + continue; + } } free(data); diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 0e9f183d499..edf7ed6454b 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -866,8 +866,10 @@ static DWORD CALLBACK stream_thread(void *arg) } static void handle_read_request(struct parser *filter, LONGLONG file_size, - void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) + void **buffer, size_t *buffer_size, struct wg_request *request) { + uint64_t offset = request->u.read.offset; + uint32_t size = request->u.read.size; HRESULT hr; void *data; @@ -878,7 +880,7 @@ static void handle_read_request(struct parser *filter, LONGLONG file_size, if (!array_reserve(buffer, buffer_size, size, 1)) { - wg_parser_push_data(filter->wg_parser, NULL, 0); + wg_parser_reply_read(filter->wg_parser, NULL, 0); return; } data = *buffer; @@ -891,7 +893,7 @@ static void handle_read_request(struct parser *filter, LONGLONG file_size, data = NULL; } - wg_parser_push_data(filter->wg_parser, data, size); + wg_parser_reply_read(filter->wg_parser, data, size); } static DWORD CALLBACK read_thread(void *arg) @@ -910,13 +912,21 @@ static DWORD CALLBACK read_thread(void *arg) while (filter->sink_connected) { - uint64_t offset; - uint32_t size; + struct wg_request request; - if (!wg_parser_get_next_read_offset(filter->wg_parser, &offset, &size)) + if (!wg_parser_wait_request(filter->wg_parser, &request)) continue; - handle_read_request(filter, file_size, &data, &buffer_size, offset, size); + switch (request.type) + { + case WG_REQUEST_READ: + handle_read_request(filter, file_size, &data, &buffer_size, &request); + break; + + default: + FIXME("Ingoring unknown request %u\n", request.type); + continue; + } } free(data); diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 860a8ab2a52..f767daed513 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -135,6 +135,24 @@ struct wg_sample BYTE *data; }; +struct wg_request +{ + enum wg_request_type + { + WG_REQUEST_NONE = 0, + WG_REQUEST_READ = 1, + } type; + + union + { + struct + { + UINT32 size; + UINT64 offset; + } read; + } u; +}; + struct wg_parser_buffer { /* pts and duration are in 100-nanosecond units. */ @@ -165,14 +183,13 @@ struct wg_parser_connect_params UINT64 file_size; }; -struct wg_parser_get_next_read_offset_params +struct wg_parser_wait_request_params { struct wg_parser *parser; - UINT32 size; - UINT64 offset; + struct wg_request request; }; -struct wg_parser_push_data_params +struct wg_parser_reply_read_params { struct wg_parser *parser; const void *data; @@ -271,8 +288,8 @@ enum unix_funcs unix_wg_parser_connect, unix_wg_parser_disconnect, - unix_wg_parser_get_next_read_offset, - unix_wg_parser_push_data, + unix_wg_parser_wait_request, + unix_wg_parser_reply_read, unix_wg_parser_get_stream_count, unix_wg_parser_get_stream, diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 865b6d3643a..56525da1348 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -77,7 +77,7 @@ struct wg_parser pthread_cond_t init_cond; bool no_more_pads, has_duration, error; - pthread_cond_t read_cond, read_done_cond; + pthread_cond_t request_cond, read_done_cond; struct { GstBuffer *buffer; @@ -126,15 +126,15 @@ static NTSTATUS wg_parser_get_stream(void *args) return S_OK; } -static NTSTATUS wg_parser_get_next_read_offset(void *args) +static NTSTATUS wg_parser_wait_request(void *args) { - struct wg_parser_get_next_read_offset_params *params = args; + struct wg_parser_wait_request_params *params = args; struct wg_parser *parser = params->parser; pthread_mutex_lock(&parser->mutex); while (parser->sink_connected && !parser->read_request.size) - pthread_cond_wait(&parser->read_cond, &parser->mutex); + pthread_cond_wait(&parser->request_cond, &parser->mutex); if (!parser->sink_connected) { @@ -142,16 +142,17 @@ static NTSTATUS wg_parser_get_next_read_offset(void *args) return VFW_E_WRONG_STATE; } - params->offset = parser->read_request.offset; - params->size = parser->read_request.size; + params->request.type = WG_REQUEST_READ; + params->request.u.read.offset = parser->read_request.offset; + params->request.u.read.size = parser->read_request.size; pthread_mutex_unlock(&parser->mutex); return S_OK; } -static NTSTATUS wg_parser_push_data(void *args) +static NTSTATUS wg_parser_reply_read(void *args) { - const struct wg_parser_push_data_params *params = args; + const struct wg_parser_reply_read_params *params = args; struct wg_parser *parser = params->parser; const void *data = params->data; uint32_t size = params->size; @@ -890,7 +891,7 @@ static GstFlowReturn src_getrange_cb(GstPad *pad, GstObject *parent, parser->read_request.offset = offset; parser->read_request.size = size; parser->read_request.done = false; - pthread_cond_signal(&parser->read_cond); + pthread_cond_signal(&parser->request_cond); /* Note that we don't unblock this wait on GST_EVENT_FLUSH_START. We expect * the upstream pin to flush if necessary. We should never be blocked on @@ -1328,7 +1329,7 @@ out: pthread_mutex_lock(&parser->mutex); parser->sink_connected = false; pthread_mutex_unlock(&parser->mutex); - pthread_cond_signal(&parser->read_cond); + pthread_cond_signal(&parser->request_cond); return E_FAIL; } @@ -1355,7 +1356,7 @@ static NTSTATUS wg_parser_disconnect(void *args) pthread_mutex_lock(&parser->mutex); parser->sink_connected = false; pthread_mutex_unlock(&parser->mutex); - pthread_cond_signal(&parser->read_cond); + pthread_cond_signal(&parser->request_cond); for (i = 0; i < parser->stream_count; ++i) free_stream(parser->streams[i]); @@ -1557,7 +1558,7 @@ static NTSTATUS wg_parser_create(void *args) pthread_mutex_init(&parser->mutex, NULL); pthread_cond_init(&parser->init_cond, NULL); - pthread_cond_init(&parser->read_cond, NULL); + pthread_cond_init(&parser->request_cond, NULL); pthread_cond_init(&parser->read_done_cond, NULL); parser->init_gst = init_funcs[params->type]; parser->unlimited_buffering = params->unlimited_buffering; @@ -1579,7 +1580,7 @@ static NTSTATUS wg_parser_destroy(void *args) pthread_mutex_destroy(&parser->mutex); pthread_cond_destroy(&parser->init_cond); - pthread_cond_destroy(&parser->read_cond); + pthread_cond_destroy(&parser->request_cond); pthread_cond_destroy(&parser->read_done_cond); free(parser); @@ -1595,8 +1596,8 @@ const unixlib_entry_t __wine_unix_call_funcs[] = X(wg_parser_connect), X(wg_parser_disconnect), - X(wg_parser_get_next_read_offset), - X(wg_parser_push_data), + X(wg_parser_wait_request), + X(wg_parser_reply_read), X(wg_parser_get_stream_count), X(wg_parser_get_stream), diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 9d1eb2a95ed..9de7ab6edf3 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -527,9 +527,11 @@ static const IWMMediaPropsVtbl stream_props_vtbl = }; static void handle_read_request(struct wm_reader *reader, uint64_t file_size, - void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) + void **buffer, size_t *buffer_size, struct wg_request *request) { + uint64_t offset = request->u.read.offset; IStream *stream = reader->source_stream; + uint32_t size = request->u.read.size; LARGE_INTEGER large_offset; HANDLE file = reader->file; ULONG ret_size = 0; @@ -543,7 +545,7 @@ static void handle_read_request(struct wm_reader *reader, uint64_t file_size, if (!array_reserve(buffer, buffer_size, size, 1)) { - wg_parser_push_data(reader->wg_parser, NULL, 0); + wg_parser_reply_read(reader->wg_parser, NULL, 0); return; } data = *buffer; @@ -576,7 +578,7 @@ static void handle_read_request(struct wm_reader *reader, uint64_t file_size, size = ret_size; } - wg_parser_push_data(reader->wg_parser, data, size); + wg_parser_reply_read(reader->wg_parser, data, size); } static DWORD CALLBACK read_thread(void *arg) @@ -610,13 +612,21 @@ static DWORD CALLBACK read_thread(void *arg) while (!reader->read_thread_shutdown) { - uint64_t offset; - uint32_t size; + struct wg_request request; - if (!wg_parser_get_next_read_offset(reader->wg_parser, &offset, &size)) + if (!wg_parser_wait_request(reader->wg_parser, &request)) continue; - handle_read_request(reader, file_size, &data, &buffer_size, offset, size); + switch (request.type) + { + case WG_REQUEST_READ: + handle_read_request(reader, file_size, &data, &buffer_size, &request); + break; + + default: + FIXME("Ingoring unknown request %u\n", request.type); + continue; + } } free(data); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/242
On 6/14/22 02:27, Rémi Bernon wrote:
static void handle_read_request(struct media_source *source, QWORD file_size, - void **buffer, size_t *buffer_size, uint64_t offset, uint32_t size) + void **buffer, size_t *buffer_size, struct wg_request *request)
This can be constified, right?
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 860a8ab2a52..f767daed513 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -135,6 +135,24 @@ struct wg_sample BYTE *data; };
+struct wg_request +{ + enum wg_request_type + { + WG_REQUEST_NONE = 0, + WG_REQUEST_READ = 1, + } type; + + union + { + struct + { + UINT32 size; + UINT64 offset; + } read; + } u; +}; + struct wg_parser_buffer { /* pts and duration are in 100-nanosecond units. */
What's the purpose of WG_REQUEST_NONE?
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/gst_private.h | 3 ++- dlls/winegstreamer/main.c | 7 +++--- dlls/winegstreamer/media_source.c | 25 +++++++++------------- dlls/winegstreamer/quartz_parser.c | 26 +++++++++-------------- dlls/winegstreamer/unixlib.h | 3 +-- dlls/winegstreamer/wg_parser.c | 14 ++++++------ dlls/winegstreamer/wg_sample.c | 34 ++++++++++++++++++++++++++++++ dlls/winegstreamer/wm_reader.c | 27 ++++++++++-------------- 8 files changed, 78 insertions(+), 61 deletions(-) diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 2552e1d7f61..9974090ecf2 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -77,7 +77,7 @@ HRESULT wg_parser_connect(struct wg_parser *parser, uint64_t file_size); void wg_parser_disconnect(struct wg_parser *parser); bool wg_parser_wait_request(struct wg_parser *parser, struct wg_request *request); -void wg_parser_reply_read(struct wg_parser *parser, const void *data, uint32_t size); +void wg_parser_reply_read(struct wg_parser *parser, struct wg_sample *sample); uint32_t wg_parser_get_stream_count(struct wg_parser *parser); struct wg_parser_stream *wg_parser_get_stream(struct wg_parser *parser, uint32_t index); @@ -123,6 +123,7 @@ extern HRESULT mfplat_DllRegisterServer(void); IMFMediaType *mf_media_type_from_wg_format(const struct wg_format *format); void mf_media_type_to_wg_format(IMFMediaType *type, struct wg_format *format); +HRESULT wg_sample_create_raw(UINT32 size, struct wg_sample **out); HRESULT wg_sample_create_mf(IMFSample *sample, struct wg_sample **out); HRESULT wg_sample_create_quartz(IMediaSample *sample, struct wg_sample **out); void wg_sample_release(struct wg_sample *wg_sample); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 25f0c38b700..0beae8f4b31 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -122,16 +122,15 @@ bool wg_parser_wait_request(struct wg_parser *parser, struct wg_request *wg_requ return true; } -void wg_parser_reply_read(struct wg_parser *parser, const void *data, uint32_t size) +void wg_parser_reply_read(struct wg_parser *parser, struct wg_sample *sample) { struct wg_parser_reply_read_params params = { .parser = parser, - .data = data, - .size = size, + .sample = sample, }; - TRACE("parser %p, data %p, size %u.\n", parser, data, size); + TRACE("parser %p, sample %p.\n", parser, sample); __wine_unix_call(unix_handle, unix_wg_parser_reply_read, ¶ms); } diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 5b98c90a8b4..401ad22c06b 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -593,26 +593,25 @@ static const IMFAsyncCallbackVtbl source_async_commands_callback_vtbl = }; static void handle_read_request(struct media_source *source, QWORD file_size, - void **buffer, size_t *buffer_size, struct wg_request *request) + struct wg_request *request) { IMFByteStream *byte_stream = source->byte_stream; uint64_t offset = request->u.read.offset; uint32_t size = request->u.read.size; + struct wg_sample *wg_sample; ULONG ret_size = 0; HRESULT hr; - void *data; if (offset >= file_size) size = 0; else if (offset + size >= file_size) size = file_size - offset; - if (!array_reserve(buffer, buffer_size, size, 1)) + if (FAILED(wg_sample_create_raw(size, &wg_sample))) { - wg_parser_reply_read(source->wg_parser, NULL, 0); + wg_parser_reply_read(source->wg_parser, NULL); return; } - data = *buffer; /* Some IMFByteStreams (including the standard file-based stream) return * an error when reading past the file size. */ @@ -620,12 +619,12 @@ static void handle_read_request(struct media_source *source, QWORD file_size, if (!size) hr = S_OK; else if (SUCCEEDED(hr = IMFByteStream_SetCurrentPosition(byte_stream, offset))) - hr = IMFByteStream_Read(byte_stream, data, size, &ret_size); + hr = IMFByteStream_Read(byte_stream, wg_sample->data, size, &ret_size); if (FAILED(hr)) { ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); - data = NULL; + wg_sample->data = NULL; } else if (ret_size != size) { @@ -633,19 +632,16 @@ static void handle_read_request(struct media_source *source, QWORD file_size, size = ret_size; } - wg_parser_reply_read(source->wg_parser, data, size); + wg_sample->size = size; + wg_parser_reply_read(source->wg_parser, wg_sample); + wg_sample_release(wg_sample); } static DWORD CALLBACK read_thread(void *arg) { struct media_source *source = arg; IMFByteStream *byte_stream = source->byte_stream; - size_t buffer_size = 4096; QWORD file_size; - void *data; - - if (!(data = malloc(buffer_size))) - return 0; IMFByteStream_GetLength(byte_stream, &file_size); @@ -661,7 +657,7 @@ static DWORD CALLBACK read_thread(void *arg) switch (request.type) { case WG_REQUEST_READ: - handle_read_request(source, file_size, &data, &buffer_size, &request); + handle_read_request(source, file_size, &request); break; default: @@ -670,7 +666,6 @@ static DWORD CALLBACK read_thread(void *arg) } } - free(data); TRACE("Media source is shutting down; exiting.\n"); return 0; } diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index edf7ed6454b..dc2be99d5e3 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -866,45 +866,40 @@ static DWORD CALLBACK stream_thread(void *arg) } static void handle_read_request(struct parser *filter, LONGLONG file_size, - void **buffer, size_t *buffer_size, struct wg_request *request) + struct wg_request *request) { uint64_t offset = request->u.read.offset; uint32_t size = request->u.read.size; + struct wg_sample *wg_sample; HRESULT hr; - void *data; if (offset >= file_size) size = 0; else if (offset + size >= file_size) size = file_size - offset; - if (!array_reserve(buffer, buffer_size, size, 1)) + if (FAILED(wg_sample_create_raw(size, &wg_sample))) { - wg_parser_reply_read(filter->wg_parser, NULL, 0); + wg_parser_reply_read(filter->wg_parser, NULL); return; } - data = *buffer; - - hr = IAsyncReader_SyncRead(filter->reader, offset, size, data); + hr = IAsyncReader_SyncRead(filter->reader, offset, size, wg_sample->data); if (FAILED(hr)) { ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); - data = NULL; + wg_sample->data = NULL; } - wg_parser_reply_read(filter->wg_parser, data, size); + wg_sample->size = size; + wg_parser_reply_read(filter->wg_parser, wg_sample); + wg_sample_release(wg_sample); } static DWORD CALLBACK read_thread(void *arg) { struct parser *filter = arg; LONGLONG file_size, unused; - size_t buffer_size = 4096; - void *data; - - if (!(data = malloc(buffer_size))) - return 0; IAsyncReader_Length(filter->reader, &file_size, &unused); @@ -920,7 +915,7 @@ static DWORD CALLBACK read_thread(void *arg) switch (request.type) { case WG_REQUEST_READ: - handle_read_request(filter, file_size, &data, &buffer_size, &request); + handle_read_request(filter, file_size, &request); break; default: @@ -929,7 +924,6 @@ static DWORD CALLBACK read_thread(void *arg) } } - free(data); TRACE("Streaming stopped; exiting.\n"); return 0; } diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f767daed513..a621ea9d4bf 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -192,8 +192,7 @@ struct wg_parser_wait_request_params struct wg_parser_reply_read_params { struct wg_parser *parser; - const void *data; - UINT32 size; + struct wg_sample *sample; }; struct wg_parser_get_stream_count_params diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 56525da1348..ee3ed1807d2 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -154,21 +154,20 @@ static NTSTATUS wg_parser_reply_read(void *args) { const struct wg_parser_reply_read_params *params = args; struct wg_parser *parser = params->parser; - const void *data = params->data; - uint32_t size = params->size; + struct wg_sample *sample = params->sample; GstBuffer *buffer = NULL; GstFlowReturn result; - if (!data) + if (!sample || !sample->data) result = GST_FLOW_ERROR; - else if (!size) + else if (!sample->size) result = GST_FLOW_EOS; - else if (!(buffer = gst_buffer_new_and_alloc(size))) + else if (!(buffer = gst_buffer_new_and_alloc(sample->size))) result = GST_FLOW_ERROR; else { - gst_buffer_fill(buffer, 0, data, size); - GST_INFO("Copied %u bytes from data %p to buffer %p", size, data, buffer); + gst_buffer_fill(buffer, 0, sample->data, sample->size); + GST_INFO("Copied %u bytes from sample %p to buffer %p", sample->size, sample, buffer); result = GST_FLOW_OK; } @@ -901,6 +900,7 @@ static GstFlowReturn src_getrange_cb(GstPad *pad, GstObject *parent, pthread_cond_wait(&parser->read_done_cond, &parser->mutex); *buffer = parser->read_request.buffer; + parser->read_request.buffer = NULL; ret = parser->read_request.ret; pthread_mutex_unlock(&parser->mutex); diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c index ae8a2d4d0c7..eb46982fabf 100644 --- a/dlls/winegstreamer/wg_sample.c +++ b/dlls/winegstreamer/wg_sample.c @@ -47,6 +47,11 @@ struct sample union { + struct + { + void *__pad[3]; + BYTE buffer[]; + } raw; struct { IMFSample *sample; @@ -59,6 +64,35 @@ struct sample } u; }; +C_ASSERT(sizeof(struct sample) == offsetof(struct sample, u.raw.buffer[0])); + +static void raw_sample_destroy(struct wg_sample *wg_sample) +{ + TRACE("wg_sample %p\n", wg_sample); +} + +static const struct wg_sample_ops raw_sample_ops = +{ + raw_sample_destroy, +}; + +HRESULT wg_sample_create_raw(UINT32 size, struct wg_sample **out) +{ + struct sample *sample; + + if (!(sample = calloc(1, offsetof(struct sample, u.raw.buffer[size])))) + return E_OUTOFMEMORY; + + sample->wg_sample.data = sample->u.raw.buffer; + sample->wg_sample.size = 0; + sample->wg_sample.max_size = size; + sample->ops = &raw_sample_ops; + + TRACE("Created wg_sample %p, size %u.\n", &sample->wg_sample, size); + *out = &sample->wg_sample; + return S_OK; +} + static const struct wg_sample_ops mf_sample_ops; static inline struct sample *unsafe_mf_from_wg_sample(struct wg_sample *wg_sample) diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 9de7ab6edf3..a2851f8df31 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -527,28 +527,27 @@ static const IWMMediaPropsVtbl stream_props_vtbl = }; static void handle_read_request(struct wm_reader *reader, uint64_t file_size, - void **buffer, size_t *buffer_size, struct wg_request *request) + struct wg_request *request) { uint64_t offset = request->u.read.offset; IStream *stream = reader->source_stream; uint32_t size = request->u.read.size; + struct wg_sample *wg_sample; LARGE_INTEGER large_offset; HANDLE file = reader->file; ULONG ret_size = 0; HRESULT hr; - void *data; if (offset >= file_size) size = 0; else if (offset + size >= file_size) size = file_size - offset; - if (!array_reserve(buffer, buffer_size, size, 1)) + if (FAILED(wg_sample_create_raw(size, &wg_sample))) { - wg_parser_reply_read(reader->wg_parser, NULL, 0); + wg_parser_reply_read(reader->wg_parser, NULL); return; } - data = *buffer; large_offset.QuadPart = offset; if (!size) @@ -556,7 +555,7 @@ static void handle_read_request(struct wm_reader *reader, uint64_t file_size, else if (file) { if (!SetFilePointerEx(file, large_offset, NULL, FILE_BEGIN) - || !ReadFile(file, data, size, &ret_size, NULL)) + || !ReadFile(file, wg_sample->data, size, &ret_size, NULL)) hr = HRESULT_FROM_WIN32(GetLastError()); else hr = S_OK; @@ -564,13 +563,13 @@ static void handle_read_request(struct wm_reader *reader, uint64_t file_size, else { if (SUCCEEDED(hr = IStream_Seek(stream, large_offset, STREAM_SEEK_SET, NULL))) - hr = IStream_Read(stream, data, size, &ret_size); + hr = IStream_Read(stream, wg_sample->data, size, &ret_size); } if (FAILED(hr)) { ERR("Failed to read %u bytes at offset %I64u, hr %#lx.\n", size, offset, hr); - data = NULL; + wg_sample->data = NULL; } else if (ret_size != size) { @@ -578,7 +577,9 @@ static void handle_read_request(struct wm_reader *reader, uint64_t file_size, size = ret_size; } - wg_parser_reply_read(reader->wg_parser, data, size); + wg_sample->size = size; + wg_parser_reply_read(reader->wg_parser, wg_sample); + wg_sample_release(wg_sample); } static DWORD CALLBACK read_thread(void *arg) @@ -586,12 +587,7 @@ static DWORD CALLBACK read_thread(void *arg) struct wm_reader *reader = arg; IStream *stream = reader->source_stream; HANDLE file = reader->file; - size_t buffer_size = 4096; uint64_t file_size; - void *data; - - if (!(data = malloc(buffer_size))) - return 0; if (file) { @@ -620,7 +616,7 @@ static DWORD CALLBACK read_thread(void *arg) switch (request.type) { case WG_REQUEST_READ: - handle_read_request(reader, file_size, &data, &buffer_size, &request); + handle_read_request(reader, file_size, &request); break; default: @@ -629,7 +625,6 @@ static DWORD CALLBACK read_thread(void *arg) } } - free(data); TRACE("Reader is shutting down; exiting.\n"); return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/242
On 6/14/22 02:27, Rémi Bernon wrote:
diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c index ae8a2d4d0c7..eb46982fabf 100644 --- a/dlls/winegstreamer/wg_sample.c +++ b/dlls/winegstreamer/wg_sample.c @@ -47,6 +47,11 @@ struct sample
union { + struct + { + void *__pad[3]; + BYTE buffer[]; + } raw; struct { IMFSample *sample;
Why the padding?
+HRESULT wg_sample_create_raw(UINT32 size, struct wg_sample **out) +{ + struct sample *sample; + + if (!(sample = calloc(1, offsetof(struct sample, u.raw.buffer[size])))) + return E_OUTOFMEMORY; + + sample->wg_sample.data = sample->u.raw.buffer; + sample->wg_sample.size = 0; + sample->wg_sample.max_size = size; + sample->ops = &raw_sample_ops; + + TRACE("Created wg_sample %p, size %u.\n", &sample->wg_sample, size); + *out = &sample->wg_sample; + return S_OK; +}
It strikes me as unnecessary to return HRESULT from this function, especially considering you're not actually using the HRESULT value (only testing for failure). It'd be simpler just to return "sample" in that case.
On Thu Jun 16 00:25:27 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 6/14/22 02:26, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
It is invalid according to GStreamer documentation. What documentation are you referring to? The documentation for GstPadGetRangeFunction itself is somewhat badly written and doesn't actually mention what "buffer" is expected to contain on input, but the documentation for gst_pad_get_range() says: If buffer points to a variable holding NULL, a valid new GstBuffer will be placed in buffer when this function returns GST_FLOW_OK. The new buffer must be freed with gst_buffer_unref after usage. When buffer points to a variable that points to a valid GstBuffer, the buffer will be filled with the result data when this function returns GST_FLOW_OK. If the provided buffer is larger than size, only size bytes will be filled in the result buffer and its size will be updated accordingly. and the implementation shows that the intent is for the same to apply to GstPadGetRangeFunction. For what it's worth, I believe oggdemux actually does pass a non-NULL buffer, and breaks if we don't fill that.
I was referring to `If this function returns GST_FLOW_OK, the result buffer will be stored in buffer. The contents of buffer is invalid for any other return value.`, but looking at `gst_pad_get_range` and `gst_pad_pull_range` source, I believe you're right...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/242#note_2176
diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c index ae8a2d4d0c7..eb46982fabf 100644 --- a/dlls/winegstreamer/wg_sample.c +++ b/dlls/winegstreamer/wg_sample.c @@ -47,6 +47,11 @@ struct sample
union { + struct + { + void *__pad[3]; + BYTE buffer[]; + } raw; struct { IMFSample *sample;
Why the padding?
So that buffer is aligned to the end of the structure and so that we can safely use a flexible array member. Otherwise offsetof buffer[size] may be shorter than the struct, ending up with a undefined behavior when accessing a partially allocated structure and an eventual GCC warning.
+HRESULT wg_sample_create_raw(UINT32 size, struct wg_sample **out) +{ + struct sample *sample; + + if (!(sample = calloc(1, offsetof(struct sample, u.raw.buffer[size])))) + return E_OUTOFMEMORY; + + sample->wg_sample.data = sample->u.raw.buffer; + sample->wg_sample.size = 0; + sample->wg_sample.max_size = size; + sample->ops = &raw_sample_ops; + + TRACE("Created wg_sample %p, size %u.\n", &sample->wg_sample, size); + *out = &sample->wg_sample; + return S_OK; +}
It strikes me as unnecessary to return HRESULT from this function, especially considering you're not actually using the HRESULT value (only testing for failure). It'd be simpler just to return "sample" in that case.
It makes it consistent with the other sample creation functions, and other creations functions in general. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/242#note_2177
On 6/16/22 01:58, Rémi Bernon (@rbernon) wrote:
diff --git a/dlls/winegstreamer/wg_sample.c b/dlls/winegstreamer/wg_sample.c index ae8a2d4d0c7..eb46982fabf 100644 --- a/dlls/winegstreamer/wg_sample.c +++ b/dlls/winegstreamer/wg_sample.c @@ -47,6 +47,11 @@ struct sample
union { + struct + { + void *__pad[3]; + BYTE buffer[]; + } raw; struct { IMFSample *sample;
Why the padding?
So that buffer is aligned to the end of the structure and so that we can safely use a flexible array member. Otherwise offsetof buffer[size] may be shorter than the struct, ending up with a undefined behavior when accessing a partially allocated structure and an eventual GCC warning.
Okay, that seems like a reasonable workaround; can you please add a comment so it's clear?
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/242
Closing for now as it'll need some rework. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/242#note_2705
participants (2)
-
Rémi Bernon -
Zebediah Figura