[PATCH 0/1] MR6629: winegstreamer: Split large WMA samples.
Replicate native behavior. This fixes Giana Sister: Twisted Dreams crashing during the intro video. The game expects to be able to fit a few audio samples in a fixed size memory area. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629
From: Matteo Bruni <mbruni(a)codeweavers.com> Replicate native behavior. This fixes Giana Sister: Twisted Dreams crashing during the intro video. The game expects to be able to fit a few audio samples in a fixed size memory area. --- dlls/winegstreamer/wm_reader.c | 54 ++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index e3016b0918b..ae9f3aed337 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -32,6 +32,9 @@ struct wm_stream bool eos; bool read_compressed; + struct wg_parser_buffer current_buffer; + DWORD current_buffer_offset; + IWMReaderAllocatorEx *output_allocator; IWMReaderAllocatorEx *stream_allocator; }; @@ -1704,9 +1707,13 @@ static HRESULT wm_reader_read_stream_sample(struct wm_reader *reader, struct wg_ TRACE("Got buffer for '%s' stream %p.\n", get_major_type_string(stream->format.major_type), stream); - if (FAILED(hr = wm_stream_allocate_sample(stream, buffer->size, sample))) + capacity = buffer->size - stream->current_buffer_offset; + if (stream->format.major_type == WG_MAJOR_TYPE_AUDIO) + capacity = min(capacity, 17208); + + if (FAILED(hr = wm_stream_allocate_sample(stream, capacity, sample))) { - ERR("Failed to allocate sample of %u bytes, hr %#lx.\n", buffer->size, hr); + ERR("Failed to allocate sample of %lu bytes, hr %#lx.\n", capacity, hr); wg_parser_stream_release_buffer(stream->wg_stream); return hr; } @@ -1715,10 +1722,9 @@ static HRESULT wm_reader_read_stream_sample(struct wm_reader *reader, struct wg_ ERR("Failed to get data pointer, hr %#lx.\n", hr); if (FAILED(hr = INSSBuffer_GetMaxLength(*sample, &capacity))) ERR("Failed to get capacity, hr %#lx.\n", hr); - if (buffer->size > capacity) - ERR("Returned capacity %lu is less than requested capacity %u.\n", capacity, buffer->size); - if (!wg_parser_stream_copy_buffer(stream->wg_stream, data, 0, buffer->size)) + size = min(buffer->size - stream->current_buffer_offset, capacity); + if (!wg_parser_stream_copy_buffer(stream->wg_stream, data, stream->current_buffer_offset, size)) { /* The GStreamer pin has been flushed. */ INSSBuffer_Release(*sample); @@ -1726,13 +1732,11 @@ static HRESULT wm_reader_read_stream_sample(struct wm_reader *reader, struct wg_ return S_FALSE; } - if (FAILED(hr = INSSBuffer_SetLength(*sample, buffer->size))) - ERR("Failed to set size %u, hr %#lx.\n", buffer->size, hr); + if (FAILED(hr = INSSBuffer_SetLength(*sample, size))) + ERR("Failed to set size %lu, hr %#lx.\n", size, hr); - wg_parser_stream_release_buffer(stream->wg_stream); - - *pts = buffer->pts; - *duration = buffer->duration; + *pts = buffer->pts + buffer->duration * (uint64_t)stream->current_buffer_offset / buffer->size; + *duration = buffer->duration * (uint64_t)size / buffer->size; if (!buffer->has_pts) { @@ -1751,6 +1755,20 @@ static HRESULT wm_reader_read_stream_sample(struct wm_reader *reader, struct wg_ if (!buffer->delta) *flags |= WM_SF_CLEANPOINT; + stream->current_buffer_offset += size; + if (stream->current_buffer_offset == buffer->size) + { + TRACE("Current buffer exhausted, releasing.\n"); + stream->current_buffer.size = 0; + stream->current_buffer_offset = 0; + wg_parser_stream_release_buffer(stream->wg_stream); + } + else + { + TRACE("Keeping buffer around, %lu bytes still available.\n", buffer->size - stream->current_buffer_offset); + stream->current_buffer = *buffer; + } + return S_OK; } @@ -1953,9 +1971,19 @@ static HRESULT WINAPI reader_GetNextSample(IWMSyncReader2 *iface, while (hr == S_FALSE) { struct wg_parser_buffer wg_buffer; - if (!wg_parser_stream_get_buffer(reader->wg_parser, stream ? stream->wg_stream : 0, &wg_buffer)) + + if (stream && stream->current_buffer.size) + { + TRACE("Reusing buffer for stream %p.\n", stream); + wg_buffer = stream->current_buffer; + hr = S_OK; + } + else if (!wg_parser_stream_get_buffer(reader->wg_parser, stream ? stream->wg_stream : 0, &wg_buffer)) + { hr = NS_E_NO_MORE_SAMPLES; - else if (SUCCEEDED(hr = wm_reader_read_stream_sample(reader, &wg_buffer, sample, pts, duration, flags))) + } + + if (SUCCEEDED(hr) && SUCCEEDED(hr = wm_reader_read_stream_sample(reader, &wg_buffer, sample, pts, duration, flags))) stream_number = wg_buffer.stream + 1; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6629
Alex Henrie (@alexhenrie) commented about dlls/winegstreamer/wm_reader.c:
TRACE("Got buffer for '%s' stream %p.\n", get_major_type_string(stream->format.major_type), stream);
- if (FAILED(hr = wm_stream_allocate_sample(stream, buffer->size, sample))) + capacity = buffer->size - stream->current_buffer_offset; + if (stream->format.major_type == WG_MAJOR_TYPE_AUDIO) + capacity = min(capacity, 17208);
Where did the number 17208 come from? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_84249
I'm not sure if that is the proper place, or way, to split samples. Also I couldn't find a way to make a sensible Wine test for this. AFAIU you can only get the larger sample sizes when using variable block length encoding, which ffmpeg (or other free encoders that I could find) doesn't support. I did hack `test_sync_reader_streaming()` to check what happens on Windows with the game video, and that's where I got the max capacity value I use in the patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_84250
On Mon Oct 7 17:16:15 2024 +0000, Alex Henrie wrote:
Where did the number 17208 come from? Heh, I was just typing that in my other comment.
I should mention that the way I'm splitting samples is somewhat different from Windows: there you get 17208 bytes capacity samples but sample size is 16384 bytes. So maybe that's a point for using 16384 as maximum capacity. I'm going to wait for more comments before touching this though :slight_smile: -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_84251
I think the WM reader should use WMA/WMV decoders DMO internally, and these would make sure the buffer sizes are the expected sizes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_84298
there you get 17208 bytes capacity samples but sample size is 16384 bytes.
Sorry, I'm not sure I understand—do you mean lSampleSize is 16384, but Windows sometimes gives you 17208 anyway? What's the offending size from the relevant game? Do we know what sizes Windows gives there? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85231
Elizabeth Figura (@zfigura) commented about dlls/winegstreamer/wm_reader.c:
ERR("Failed to get data pointer, hr %#lx.\n", hr); if (FAILED(hr = INSSBuffer_GetMaxLength(*sample, &capacity))) ERR("Failed to get capacity, hr %#lx.\n", hr); - if (buffer->size > capacity) - ERR("Returned capacity %lu is less than requested capacity %u.\n", capacity, buffer->size);
Well, this check was kind of supposed to be there as a sanity check, since we're asking the application for a buffer of the relevant size. I don't mind calling it excessive and removing it, but then we don't really need the GetMaxLength() either. Either way "capacity" is kind of a weird name for the actual content size we're going to return. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85235
On Thu Oct 17 00:42:51 2024 +0000, Elizabeth Figura wrote:
Well, this check was kind of supposed to be there as a sanity check, since we're asking the application for a buffer of the relevant size. I don't mind calling it excessive and removing it, but then we don't really need the GetMaxLength() either. Either way "capacity" is kind of a weird name for the actual content size we're going to return. Eh, I misread a bit what's going on here. Is the application actually returning a buffer that's smaller than we request, or is it just unable to handle a request that's too large? These kind of seem like two separate changes, and I'm not sure there's a point changing the ERR to "proper" handling here (is it really proper handling?) if it doesn't per se help.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85236
We need to clean up the buffer before destroying the parser now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85237
On Thu Oct 17 00:41:09 2024 +0000, Elizabeth Figura wrote:
there you get 17208 bytes capacity samples but sample size is 16384 bytes. Sorry, I'm not sure I understand—do you mean lSampleSize is 16384, but Windows sometimes gives you 17208 anyway? What's the offending size from the relevant game? Do we know what sizes Windows gives there? I think Mystral is saying that the capacity (INSSBuffer::GetMaxLength) is 17208 and the actual size (INSSBuffer::GetLength) is 16384. lSampleSize is a AM_MEDIA_TYPE member, which (as far as I can see) isn't relevant to this function.
Personally I'd go for matching native's size, not capacity, and use 16384. Partially because consumers are more likely to use size than capacity, partially because 17208 is ugly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85238
On Thu Oct 17 00:45:51 2024 +0000, Elizabeth Figura wrote:
Eh, I misread a bit what's going on here. Is the application actually returning a buffer that's smaller than we request, or is it just unable to handle a request that's too large? These kind of seem like two separate changes, and I'm not sure there's a point changing the ERR to "proper" handling here (is it really proper handling?) if it doesn't per se help. If I'm reading this thread correctly, the app isn't returning anything. We're the ones returning things, and the app gets nervous if we return too much at once.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85239
lSampleSize is a AM_MEDIA_TYPE member, which (as far as I can see) isn't relevant to this function.
AM_MEDIA_TYPE or the synonymous WM_MEDIA_TYPE; I was referring to the type set on the stream in question. I gather the application is confused by the size, not capacity, in which case, yes, we should be clamping to native's size, but I'm not sure that's what Matteo meant. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85243
On Thu Oct 17 00:51:54 2024 +0000, Alfred Agrell wrote:
If I'm reading this thread correctly, the app isn't returning anything. We're the ones returning things, and the app gets nervous if we return too much at once. You can register a callback to allocate buffers; that's the point of this code and this sanity check. If there's no such callback then we allocate them ourselves.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85244
On Thu Oct 17 01:13:41 2024 +0000, Elizabeth Figura wrote:
You can register a callback to allocate buffers; that's the point of this code and this sanity check. If there's no such callback then we allocate them ourselves. If I recall correctly, the application doesn't set any custom allocator, so we get "default" allocation behavior.
What happens for the game (and identically if you e.g. hack the relevant .wmv video from the game into wmvcore/tests/wmvcore.c:test_sync_reader_streaming()) is that all the output audio samples have size (in the `INSSBuffer_GetBufferAndLength(sample, &data, &size);` sense) between 8KiB and 16KiB and "capacity" (as in `INSSBuffer_GetMaxLength(sample, &capacity);`) 17208. So that means, as I understand it, that native is doing something more complicated than what this patch does, but the general effect is more or less the same. I could restore that sanity check (but checking with the `size` value I compute immediately after), it's just that it seems somewhat redundant. We just asked for a buffer large at least `capacity` and got a non-`FAILED(hr)` response, I'd be quite surprised if whatever serviced the allocation request lied to us. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85285
On Thu Oct 17 09:35:06 2024 +0000, Elizabeth Figura wrote:
We need to clean up the buffer before destroying the parser now. Ah yep, we do. Thanks!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85286
I could restore that sanity check (but checking with the `size` value I compute immediately after), it's just that it seems somewhat redundant. We just asked for a buffer large at least `capacity` and got a non-`FAILED(hr)` response, I'd be quite surprised if whatever serviced the allocation request lied to us.
Yeah, Hence like I said—I don't mind calling it excessive and removing it, but then we don't really need the GetMaxLength() either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85344
On Thu Oct 17 01:12:24 2024 +0000, Elizabeth Figura wrote:
lSampleSize is a AM_MEDIA_TYPE member, which (as far as I can see) isn't relevant to this function. AM_MEDIA_TYPE or the synonymous WM_MEDIA_TYPE; I was referring to the type set on the stream in question. I gather the application is confused by the size, not capacity, in which case, yes, we should be clamping to native's size, but I'm not sure that's what Matteo meant. Yeah, agreed this should clamp to 16KiB. As I mentioned in the other comment, that's not quite the same as Windows but it's apparently good enough in that, with this patch, the samples fit into the fixed-size memory area.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85348
On Thu Oct 17 16:47:20 2024 +0000, Elizabeth Figura wrote:
I could restore that sanity check (but checking with the `size` value I compute immediately after), it's just that it seems somewhat redundant. We just asked for a buffer large at least `capacity` and got a non-`FAILED(hr)` response, I'd be quite surprised if whatever serviced the allocation request lied to us. Yeah, Hence like I said—I don't mind calling it excessive and removing it, but then we don't really need the GetMaxLength() either. Ah yes, fair enough.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6629#note_85349
participants (6)
-
Alex Henrie (＠alexhenrie) -
Alfred Agrell (＠Alcaro)
-
Elizabeth Figura (＠zfigura)
-
Matteo Bruni
-
Matteo Bruni (＠Mystral)
-
Rémi Bernon