October 2023 - Wine-GitLab - list.winehq.org

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 31, 2023

Oct. 31, 2023

On Tue Oct 31 20:09:06 2023 +0000, Zebediah Figura wrote: > Thanks, this is looking almost right now, just a few more nitpicks. > And again sorry about the late review :-/ Yeah, this project is getting way longer than I expected. With how many watchers BUG-9127 has, I'm surprised there aren't more people floating around asking and/or "asking" everyone to hurry up... but on the other hand, nobody linked this MR in that bug, so they won't know anything's going on until Alexandre closes bugs fixed in 8.20. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50386

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 31, 2023

Oct. 31, 2023

On Tue Oct 31 18:36:03 2023 +0000, Zebediah Figura wrote: > This can be static now. > Let's use enum wg_video_format instead of wg_video_format; the latter is > a typedef of UINT32 and therefore the compiler won't recognize it as an enum. > I don't particularly like the naming "get_bytes"; that sounds like it's > retrieving the actual data in some sense. How about > "wg_format_get_max_size_video_raw()", matching the naming scheme for wg_format_to_caps()? Yeah, I refactored that one about 38 times while creating this MR, some of which called it from other source files. But yeah, this version doesn't, so let's fix it. (Did anyone try compiling Wine with -Wmissing-declarations? That one throws a warning for nonstatic functions with no preceding prototype, and is perfect for catching things that should be static.) I have no particular opinion on the name, so sure, we can do that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50383

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 31, 2023

Oct. 31, 2023

On Tue Oct 31 18:36:05 2023 +0000, Zebediah Figura wrote: > Since we've implemented "mt", can we check its contents? but then the tests break on native because that thing gives weird results unless the source pins are connected :< Fixed, and then fixed the Wine-side code to match. Also fixed a buffer overflow; calling wg_parser_get_stream() before range checking the index probably isn't a particularly brilliant idea. ...it's just completely random which functions need FreeMediaType and which need DeleteMediaType, isn't it? At least it's documented. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50385

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 31, 2023

Oct. 31, 2023

On Tue Oct 31 18:36:04 2023 +0000, Zebediah Figura wrote: > We can delete this line. https://learn.microsoft.com/en-us/windows/win32/api/strmif/nf-strmif-iamstr… says that parameter can return non-NULL, so it felt wrong to potentially leak it. But that same page says this specific filter, and every Windows builtin, always returns NULL, so checking for something that's documented to never happen is also kinda wrong. Which means I have no particular opinion in either direction. If you do, let's go with that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50384

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 31, 2023

Oct. 31, 2023

On Tue Oct 31 18:36:02 2023 +0000, Zebediah Figura wrote: > We need to leave the CS on these return paths. Ah, good point. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50382

1 0

[PATCH v2 0/6] MR4224: dmsynth: Various fixes
by Anton Baskanov (＠baskanov) Oct. 31, 2023

Oct. 31, 2023

-- v2: dmsynth: Return void from add_mod_from_connection(). https://gitlab.winehq.org/wine/wine/-/merge_requests/4224

4 10

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) Oct. 31, 2023

Oct. 31, 2023

Thanks, this is looking almost right now, just a few more nitpicks. And again sorry about the late review :-/ -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50378

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) Oct. 31, 2023

Oct. 31, 2023

Zebediah Figura (@zfigura) commented about dlls/quartz/tests/mpegsplit.c: > + LPWSTR name; > + HRESULT hr; > + LCID lcid; > + > + IAMStreamSelect_QueryInterface(sel, &IID_IBaseFilter, (void **)&filter); > + IBaseFilter_FindPin(filter, L"Video", &pin_video); > + IBaseFilter_FindPin(filter, L"Audio", &pin_audio); > + IPin_QueryInterface(pin_video, &IID_IUnknown, (void **)&unk_video); > + IPin_QueryInterface(pin_audio, &IID_IUnknown, (void **)&unk_audio); > + > + hr = IAMStreamSelect_Count(sel, &stream_count); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + ok(stream_count == 2, "Got %lu streams.\n", stream_count); > + > + hr = IAMStreamSelect_Info(sel, 0, &mt, &flags, &lcid, &group, &name, &object, &unk); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); Since we've implemented "mt", can we check its contents? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50377

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) Oct. 31, 2023

Oct. 31, 2023

Zebediah Figura (@zfigura) commented about dlls/quartz/tests/mpegsplit.c: > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + todo_wine ok(flags == AMSTREAMSELECTINFO_ENABLED, "Got flags %lx.\n", flags); > + ok(lcid == 0, "Got LCID %lx.\n", lcid); > + ok(group == 0, "Got group %lx.\n", group); > + todo_wine ok(name && !wcscmp(name, L"Stream(E0)"), "Got name %ls.\n", name ? name : L"(null)"); > + todo_wine ok(object == unk_video, "Got object %p, expected %p.\n", object, unk_video); > + ok(unk == NULL, "Got unknown %p.\n", unk); > + > + if (name) > + CoTaskMemFree(name); > + if (mt) > + DeleteMediaType(mt); > + if (object) > + IUnknown_Release(object); > + if (unk) > + IUnknown_Release(unk); We can delete this line. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50376

1 0

Re: [PATCH v20 0/14] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) Oct. 31, 2023

Oct. 31, 2023

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/quartz_parser.c: > > #define ALIGN(n, alignment) (((n) + (alignment) - 1) & ~((alignment) - 1)) > > +unsigned int wg_format_get_bytes_for_uncompressed(wg_video_format format, unsigned int width, unsigned int height) This can be static now. Let's use enum wg_video_format instead of wg_video_format; the latter is a typedef of UINT32 and therefore the compiler won't recognize it as an enum. I don't particularly like the naming "get_bytes"; that sounds like it's retrieving the actual data in some sense. How about "wg_format_get_max_size_video_raw()", matching the naming scheme for wg_format_to_caps()? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_50375

1 0