[PATCH v4 0/3] MR22: winegstreamer: Implement more sample attributes and timestamps.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183 -- v4: winegstreamer: Only resize buffer when read is incomplete. winegstreamer: Support the MFSampleExtension_CleanPoint sample attribute. winegstreamer: Add timestamp and duration to struct wg_sample. https://gitlab.winehq.org/wine/wine/-/merge_requests/22
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/tests/mf.c | 6 ------ dlls/winegstreamer/mfplat.c | 17 +++++++++++++++++ dlls/winegstreamer/unixlib.h | 5 +++++ dlls/winegstreamer/wg_transform.c | 25 +++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index a9706aa7647..5abf910ad71 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6500,19 +6500,15 @@ static void test_wma_decoder(void) ok(flags == 0, "got flags %#lx\n", flags); time = 0xdeadbeef; hr = IMFSample_GetSampleTime(sample, &time); - todo_wine ok(hr == S_OK, "GetSampleTime returned %#lx\n", hr); - todo_wine ok(time == i * 928798, "got time %I64d\n", time); duration = 0xdeadbeef; hr = IMFSample_GetSampleDuration(sample, &duration); - todo_wine ok(hr == S_OK, "GetSampleDuration returned %#lx\n", hr); if (output.dwStatus == MFT_OUTPUT_DATA_BUFFER_INCOMPLETE || broken(output.dwStatus == (MFT_OUTPUT_DATA_BUFFER_INCOMPLETE|7))) { ok(length == wmadec_block_size, "got length %lu\n", length); - todo_wine ok(duration == 928798, "got duration %I64d\n", duration); check_sample_pcm16(sample, wmadec_data, output_file, TRUE); wmadec_data += wmadec_block_size; @@ -7242,9 +7238,7 @@ static void test_h264_decoder(void) /* doesn't matter what frame rate we've selected, duration is defined by the stream */ duration = 0xdeadbeef; hr = IMFSample_GetSampleDuration(output.pSample, &duration); - todo_wine ok(hr == S_OK, "GetSampleDuration returned %#lx\n", hr); - todo_wine ok(duration - 333666 <= 2, "got duration %I64d\n", duration); /* Win8 and before pad the data with garbage instead of original diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 97e27bb7301..9dcfc558963 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -903,6 +903,7 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) { DWORD current_length, max_length; struct mf_sample *mf_sample; + LONGLONG time, duration; BYTE *buffer; HRESULT hr; @@ -913,6 +914,17 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) if (FAILED(hr = IMFMediaBuffer_Lock(mf_sample->media_buffer, &buffer, &max_length, ¤t_length))) goto out; + if (SUCCEEDED(IMFSample_GetSampleTime(sample, &time))) + { + mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_PTS; + mf_sample->wg_sample.pts = time; + } + if (SUCCEEDED(IMFSample_GetSampleDuration(sample, &duration))) + { + mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_DURATION; + mf_sample->wg_sample.duration = duration; + } + IMFSample_AddRef((mf_sample->sample = sample)); mf_sample->wg_sample.data = buffer; mf_sample->wg_sample.size = current_length; @@ -937,6 +949,11 @@ void mf_destroy_wg_sample(struct wg_sample *wg_sample) IMFMediaBuffer_SetCurrentLength(mf_sample->media_buffer, wg_sample->size); IMFMediaBuffer_Release(mf_sample->media_buffer); + if (wg_sample->flags & WG_SAMPLE_FLAG_HAS_PTS) + IMFSample_SetSampleTime(mf_sample->sample, wg_sample->pts); + if (wg_sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) + IMFSample_SetSampleDuration(mf_sample->sample, wg_sample->duration); + IMFSample_Release(mf_sample->sample); free(mf_sample); } diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..32e5b068187 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -114,10 +114,15 @@ struct wg_format enum wg_sample_flag { WG_SAMPLE_FLAG_INCOMPLETE = 1, + WG_SAMPLE_FLAG_HAS_PTS = 2, + WG_SAMPLE_FLAG_HAS_DURATION = 4, }; struct wg_sample { + /* timestamp and duration are in 100-nanosecond units. */ + UINT64 pts; + UINT64 duration; UINT32 flags; UINT32 max_size; UINT32 size; diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 49c7bfaa927..afee8478b9b 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -223,6 +223,9 @@ NTSTATUS wg_transform_create(void *args) * to match its expectations. */ transform->input_max_length = 16; + if (!(element = create_element("h264parse", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out; /* fallthrough */ case WG_MAJOR_TYPE_WMA: if (!(element = transform_find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, raw_caps)) @@ -362,6 +365,10 @@ NTSTATUS wg_transform_push_data(void *args) return STATUS_NO_MEMORY; } gst_buffer_fill(buffer, 0, sample->data, sample->size); + if (sample->flags & WG_SAMPLE_FLAG_HAS_PTS) + GST_BUFFER_PTS(buffer) = sample->pts * 100; + if (sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) + GST_BUFFER_DURATION(buffer) = sample->duration * 100; gst_buffer_list_insert(transform->input, -1, buffer); GST_INFO("Copied %u bytes from sample %p to input buffer list", sample->size, sample); @@ -391,6 +398,24 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * gst_buffer_unmap(buffer, &info); gst_buffer_resize(buffer, sample->size, -1); + if (GST_BUFFER_PTS_IS_VALID(buffer)) + { + sample->flags |= WG_SAMPLE_FLAG_HAS_PTS; + sample->pts = GST_BUFFER_PTS(buffer) / 100; + } + if (GST_BUFFER_DURATION_IS_VALID(buffer)) + { + GstClockTime duration = GST_BUFFER_DURATION(buffer) / 100; + + duration = (duration * sample->size) / info.size; + GST_BUFFER_DURATION(buffer) -= duration * 100; + if (GST_BUFFER_PTS_IS_VALID(buffer)) + GST_BUFFER_PTS(buffer) += duration * 100; + + sample->flags |= WG_SAMPLE_FLAG_HAS_DURATION; + sample->duration = duration; + } + GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mf/tests/mf.c | 2 -- dlls/winegstreamer/mfplat.c | 5 +++++ dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 4 ++++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 5abf910ad71..a32005863d3 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6488,9 +6488,7 @@ static void test_wma_decoder(void) ok(status == 0, "got status %#lx\n", status); value = 0xdeadbeef; hr = IMFSample_GetUINT32(sample, &MFSampleExtension_CleanPoint, &value); - todo_wine ok(hr == S_OK, "GetUINT32 MFSampleExtension_CleanPoint returned %#lx\n", hr); - todo_wine ok(value == 1, "got MFSampleExtension_CleanPoint %u\n", value); hr = IMFSample_GetTotalLength(sample, &length); ok(hr == S_OK, "GetTotalLength returned %#lx\n", hr); diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 9dcfc558963..ee3a1f5e024 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -904,6 +904,7 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) DWORD current_length, max_length; struct mf_sample *mf_sample; LONGLONG time, duration; + UINT32 value; BYTE *buffer; HRESULT hr; @@ -924,6 +925,8 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_DURATION; mf_sample->wg_sample.duration = duration; } + if (SUCCEEDED(IMFSample_GetUINT32(sample, &MFSampleExtension_CleanPoint, &value)) && value) + mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_SYNC_POINT; IMFSample_AddRef((mf_sample->sample = sample)); mf_sample->wg_sample.data = buffer; @@ -953,6 +956,8 @@ void mf_destroy_wg_sample(struct wg_sample *wg_sample) IMFSample_SetSampleTime(mf_sample->sample, wg_sample->pts); if (wg_sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) IMFSample_SetSampleDuration(mf_sample->sample, wg_sample->duration); + if (wg_sample->flags & WG_SAMPLE_FLAG_SYNC_POINT) + IMFSample_SetUINT32(mf_sample->sample, &MFSampleExtension_CleanPoint, 1); IMFSample_Release(mf_sample->sample); free(mf_sample); diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 32e5b068187..b8dd2194bbe 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -116,6 +116,7 @@ enum wg_sample_flag WG_SAMPLE_FLAG_INCOMPLETE = 1, WG_SAMPLE_FLAG_HAS_PTS = 2, WG_SAMPLE_FLAG_HAS_DURATION = 4, + WG_SAMPLE_FLAG_SYNC_POINT = 8, }; struct wg_sample diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index afee8478b9b..02f93bd95c0 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -369,6 +369,8 @@ NTSTATUS wg_transform_push_data(void *args) GST_BUFFER_PTS(buffer) = sample->pts * 100; if (sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) GST_BUFFER_DURATION(buffer) = sample->duration * 100; + if (!(sample->flags & WG_SAMPLE_FLAG_SYNC_POINT)) + GST_BUFFER_FLAG_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT); gst_buffer_list_insert(transform->input, -1, buffer); GST_INFO("Copied %u bytes from sample %p to input buffer list", sample->size, sample); @@ -415,6 +417,8 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * sample->flags |= WG_SAMPLE_FLAG_HAS_DURATION; sample->duration = duration; } + if (!GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT)) + sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
From: Rémi Bernon <rbernon(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 02f93bd95c0..3e285e8c943 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -398,7 +398,9 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * memcpy(sample->data, info.data, sample->size); gst_buffer_unmap(buffer, &info); - gst_buffer_resize(buffer, sample->size, -1); + + if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) + gst_buffer_resize(buffer, sample->size, -1); if (GST_BUFFER_PTS_IS_VALID(buffer)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
Updated with only the hopefully straightforward changes for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/22#note_629
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/22
On Thu May 5 22:55:39 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 5/5/22 02:20, Rémi Bernon (@rbernon) wrote:
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..7a31020fb87 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -122,6 +122,7 @@ struct wg_sample UINT32 max_size; UINT32 size; BYTE *data; + struct wg_format *format; };
struct wg_parser_buffer
I was doubtful about introducing this abstraction in the first place, before we've actually implemented zero-copy, and indeed I'm finding it difficult to reason around...
Anyway, it's probably not worth restructuring things at this rate, but I have a hard time feeling like "format" belongs here. It's an in-out parameter, and as an "out" parameter it is a property of the buffer, but as an "in" parameter it isn't, really.
I think it is, in both ways. The format tells what the client believes the sample format is (even if it is uninitialized at the time it gives it to wg_transform), but it can describe what it expects in term of data size and layout for instance. If it doesn't match what we're going to give it, we return a format change notification and the new format. Maybe. It's not an immediately clear design, though, which is why I'm inclined to separate it somehow. (But see below anyway...)
Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side?
(Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?)
I don't see what benefit it would have, we need to return the information about the new format to the client anyway so that it can update the properties it exposes to the MF caller. Er, right, I forgot we still need to store the format anyway, so ignore that part :-) Still, the first part seems reasonable, unless I'm missing something? Or, frankly, we could make it output-only, and set the "format changed" flag entirely on the client side. The way things currently are, the logic is kind of split in two, and it feels awkward.
GstSample seems a much nicer high level API to me, and that it is better to use it than than something low level like gst_mini_object_set_qdata.
In either case you will need to allocate something, either the GstSample or the wg_format you keep on the buffers, because the format can change and because buffers can be queued. GstSample and caps is much cleaner imho.
In any case I'd rather avoid another complete rewrite at this point, it's been months already since I started upstreaming these patches and I would prefer to keep things like this if it's not completely backwards.
@@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + }
This looks wrong; aren't we dropping the sample data on the floor?
No? We're not releasing output_sample there.
Sorry, not dropping it on the floor exactly, but we're also not filling the buffer, whereas the mfplat code (and the tests) looks like it expects the buffer to be filled. ``` Zebediah Figura (she/her) replied on the mailing list:
Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side?
(Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?)
I don't see what benefit it would have, we need to return the information about the new format to the client anyway so that it can update the properties it exposes to the MF caller.
Er, right, I forgot we still need to store the format anyway, so ignore that part :-)
Still, the first part seems reasonable, unless I'm missing something?
Or, frankly, we could make it output-only, and set the "format changed" flag entirely on the client side. The way things currently are, the logic is kind of split in two, and it feels awkward.
There are cases, like when the application calls ProcessMessage with BEGIN_STREAMING, where we want to reset the format and receive format change notification, again. Having the stream format stored on the MF transform side lets us do that by reseting it to its default, and trigger the format change again. Call of Duty: Black Ops 3 depends on these notifications. I'll try something, but that game, and the others using the H264 have been really sensitive to tiny behavior changes.
@@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + }
This looks wrong; aren't we dropping the sample data on the floor?
No? We're not releasing output_sample there.
Sorry, not dropping it on the floor exactly, but we're also not filling the buffer, whereas the mfplat code (and the tests) looks like it expects the buffer to be filled.
The tests check that the returned sample, on format change, is empty, which should be the case, and mf_destroy_wg_sample will convert the wg sample properties back to the MF side. After a stream change you need, and can, call ProcessOutput once more, immediately, to get the sample data. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/22#note_716
On 5/6/22 02:01, Rémi Bernon (@rbernon) wrote:
Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side?
(Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?)
I don't see what benefit it would have, we need to return the information about the new format to the client anyway so that it can update the properties it exposes to the MF caller.
Er, right, I forgot we still need to store the format anyway, so ignore that part :-)
Still, the first part seems reasonable, unless I'm missing something?
Or, frankly, we could make it output-only, and set the "format changed" flag entirely on the client side. The way things currently are, the logic is kind of split in two, and it feels awkward.
There are cases, like when the application calls ProcessMessage with BEGIN_STREAMING, where we want to reset the format and receive format change notification, again. Having the stream format stored on the MF transform side lets us do that by reseting it to its default, and trigger the format change again.
Sure, then the question becomes, can we move the entire logic to the client side? That would require a separate call to retrieve the type of the current sample, given the below (or alternatively some extra queuing on the client side, but that seems more complex than necessary), but structurally it feels a lot less awkward.
Call of Duty: Black Ops 3 depends on these notifications.
I'll try something, but that game, and the others using the H264 have been really sensitive to tiny behavior changes.
@@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + }
This looks wrong; aren't we dropping the sample data on the floor?
No? We're not releasing output_sample there.
Sorry, not dropping it on the floor exactly, but we're also not filling the buffer, whereas the mfplat code (and the tests) looks like it expects the buffer to be filled.
The tests check that the returned sample, on format change, is empty, which should be the case, and mf_destroy_wg_sample will convert the wg sample properties back to the MF side.
After a stream change you need, and can, call ProcessOutput once more, immediately, to get the sample data.
Okay, I missed that. That's a bizarre API design; I don't know why they'd send an empty sample instead of no sample at all...
participants (3)
-
Rémi Bernon -
Zebediah Figura (＠zfigura)
-
Zebediah Figura (she/her)