November 2023 - Wine-GitLab

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

On Thu Nov 30 21:56:36 2023 +0000, Alfred Agrell wrote: > Because WMV version 1 doesn't have codec data. > ``` > $ gst-launch-1.0 -vvv filesrc location=dlls/wmvcore/tests/test.wmv ! > asfdemux ! avdec_wmv1 ! fakesink > [...] > /GstPipeline:pipeline0/avdec_wmv1:avdec_wmv1-0.GstPad:sink: caps = > video/x-wmv, wmvversion=(int)1, width=(int)64, height=(int)48, > pixel-aspect-ratio=(fraction)1/1, format=(string)WMV1 > $ gst-launch-1.0 -vvv filesrc location=waga.wmv ! asfdemux ! avdec_wmv3 > ! fakesink > [...] > /GstPipeline:pipeline0/avdec_wmv3:avdec_wmv3-0.GstPad:sink: caps = > video/x-wmv, wmvversion=(int)3, format=(string)WMV3, width=(int)1280, > height=(int)720, codec_data=(buffer)4ff1080100, > pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)10000000/333333 > ``` > (Its absense is tested by the cbFormat check in compare_media_types().) > But yes, certainly suboptimal. I'd be happy to test it if someone can > find a suitable test file. If you've tested manually, that's probably fine. My concern is primarily that we're actually translating formats correctly; GStreamer and DirectShow don't always store them in the same way, and it's too easy to make assumptions (cf. [1]). [1] https://gitlab.winehq.org/wine/wine/-/merge_requests/3636#note_45288 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54471

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/wmvcore/tests/wmvcore.c: > ok(callback->next_pts[1] == 13270000, "Got pts %I64d.\n", callback->next_pts[1]); > ok(callback->sample_count > 0, "Got no samples.\n"); > callback->sample_count = 0; > + ok(!callback->todo_rewind, "Didn't rewind as expected\n"); This also seems unnecessary? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54470

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/wmvcore/tests/wmvcore.c: > todo_wine > ok(hr == E_UNEXPECTED, "Got hr %#lx.\n", hr); > > + if (winetest_platform_is_wine) I don't think you need the if (winetest_platform_is_wine) here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54469

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/wmvcore/tests/wmvcore.c: > trace("%lu: %04lx: IWMReaderCallback::OnSample(output %lu, time %I64u, duration %I64u, flags %#lx)\n", > GetTickCount(), GetCurrentThreadId(), output, time, duration, flags); > > + if (callback->last_pts[output] > time && callback->todo_rewind) > + { > + callback->todo_rewind = false; > + todo_wine ok(0, "changing compression state in Wine rewinds the stream\n"); > + callback->last_pts[0] = 0; > + callback->last_pts[1] = 0; > + } > + "if (x) ok(0)" should almost always be "ok(!x)". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54468

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wm_reader.c: > > wg_parser_stream_release_buffer(stream->wg_stream); > > - if (!buffer->has_pts) > + if (buffer->has_pts) > + *pts = buffer->pts; > + else > FIXME("Missing PTS.\n"); > - if (!buffer->has_duration) > + if (buffer->has_duration) > + *duration = buffer->duration; > + else > FIXME("Missing duration.\n"); > > - *pts = buffer->pts; > - *duration = buffer->duration; This seems unrelated, and what's the motivation? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54467

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wm_reader.c: > parser = reader->wg_parser; > LeaveCriticalSection(&reader->init_cs); > > + if (!parser) > + { > + Sleep(10); > + continue; > + } > + This is ugly. Either we should explicitly block the read thread, or shut it down and restart it. The latter is probably simpler. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54465

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wm_reader.c: > > - reader->stream_count = wg_parser_get_stream_count(reader->wg_parser); > - > - if (!(reader->streams = calloc(reader->stream_count, sizeof(*reader->streams)))) > + if (!reader->streams) > { > - hr = E_OUTOFMEMORY; > - goto out_disconnect_parser; > + reader->stream_count = wg_parser_get_stream_count(reader->wg_parser); > + if (!(reader->streams = calloc(reader->stream_count, sizeof(*reader->streams)))) > + { > + hr = E_OUTOFMEMORY; > + goto out_disconnect_parser; > + } > + for (i = 0; i < reader->stream_count; ++i) > + reader->streams[i].selection = WMT_ON; I don't like this pattern; in cases like this we should instead just split up the init_stream() helper to the part that's done on initialization and the part that's done on recreation. That said I would imagine more of this should be done on initialization...? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54466

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/quartz_parser.c: > else > format->u.video_wmv.format = WG_WMV_VIDEO_FORMAT_UNKNOWN; > > + format->u.video_wmv.codec_data_len = mt->cbFormat - sizeof(VIDEOINFOHEADER); > + if (format->u.video_wmv.codec_data_len > sizeof(format->u.video_wmv.codec_data)) > + { > + WARN("Too big codec_data value (%u).\n", (UINT)format->u.video_wmv.codec_data_len); This cast should be unnecessary. I'd also make this an ERR or FIXME. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54464

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Alfred Agrell (＠Alcaro) Nov. 30, 2023

Nov. 30, 2023

On Thu Nov 30 21:42:02 2023 +0000, Zebediah Figura wrote: > The codec_data introduced in 1/6 isn't validated in the tests. Why is that? Because WMV version 1 doesn't have codec data. ``` $ gst-launch-1.0 -vvv filesrc location=dlls/wmvcore/tests/test.wmv ! asfdemux ! avdec_wmv1 ! fakesink [...] /GstPipeline:pipeline0/avdec_wmv1:avdec_wmv1-0.GstPad:sink: caps = video/x-wmv, wmvversion=(int)1, width=(int)64, height=(int)48, pixel-aspect-ratio=(fraction)1/1, format=(string)WMV1 $ gst-launch-1.0 -vvv filesrc location=waga.wmv ! asfdemux ! avdec_wmv3 ! fakesink [...] /GstPipeline:pipeline0/avdec_wmv3:avdec_wmv3-0.GstPad:sink: caps = video/x-wmv, wmvversion=(int)3, format=(string)WMV3, width=(int)1280, height=(int)720, codec_data=(buffer)4ff1080100, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)10000000/333333 ``` (Its absense is tested by the cbFormat check in compare_media_types().) But yes, certainly suboptimal. I'd be happy to test it if someone can find a suitable test file. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54462

1 0

Re: [PATCH v7 0/6] MR4449: winegstreamer: WMReader compressed output
by Zebediah Figura (＠zfigura) Nov. 30, 2023

Nov. 30, 2023

The codec_data introduced in 1/6 isn't validated in the tests. Why is that? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4449#note_54461

1 0