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

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:59 2023 +0000, Zebediah Figura wrote: > If Windows never sets the sample time for video samples after the first > then we should probably make this check more restrictive to explicitly > check that. It's Complicated™ (Apologies if you know some of this already, better too much information than too little.) The MPEG video format contains two layers of framing - the program stream tells which bytes are audio and which are video, but it's just arbitrarily arranged bytes, with no relation to video frame boundaries. Windows' MPEG splitter parses the program stream, reads the first few bytes of the video stream (the codec data) to determine image size, and emits that. Determining the frame boundaries is the video codec's job. GStreamer's mpegpsdemux doesn't read the codec data because why would it. To make the splitter's media type contain image size, I needed to put the mpegvideoparse on this side. Which means that, unlike Windows, each submitted packet is exactly one frame. ``` Windows video0 hr=00000000 len=1214 audio0 hr=00000000 len=2037 audio1 hr=00000000 len=2037 audio2 hr=00000000 len=2025 audio3 hr=00000000 len=2025 audio4 hr=80040249 len=653 Wine video0 hr=00000000 len=859 audio0 hr=00000000 len=1253 video1 hr=80040249 len=195 audio1 hr=00000000 len=1254 video2 hr=80040249 len=84 audio2 hr=00000000 len=1254 video3 hr=80040249 len=76 audio3 hr=00000000 len=1254 audio4 hr=00000000 len=1254 audio5 hr=00000000 len=1254 audio6 hr=00000000 len=1254 ``` Yes, it's messy, but I don't think we can do any better unless I move the mpegvideoparse into the codec object. Which would be more accurate to native, but it would require writing my own decoder for the codec data, would require reintroducing <https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_46588>, and would improve nothing except our test suite. That said, the VFW_S_NO_STOP_TIME check catches nothing. I'll drop it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49353

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:46:01 2023 +0000, Zebediah Figura wrote: > Shouldn't we get an exact number of frames here? Or is that not > consistent somehow? Yes, it's inconsistent. Wine emits two frames; native only emits one, probably because the graph is paused. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49354

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:56 2023 +0000, Zebediah Figura wrote: > This probably needs to take the filter CS. Same for Info() for that matter. You sure? There's no reference to filter_cs in dlls/quartz/ or dlls/winegstreamer/, except in VMR; it's mostly used in libs/strmbase/. Is this specific interface different? Or maybe they're all buggy? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49352

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:46:06 2023 +0000, Zebediah Figura wrote: > Usually enum values are capitalized; also, _t is usually reserved for > typedefs. That said, do we even want this enum? Is there any instance > where we shouldn't just iterate over the whole array? > Two of these formats are todo, but I think something like "todo_wine_if > (i == 1 || i == 8)" would be fine. Turns out that no, we don't. Every piece either should test every supported format, or uses only one format and doesn't care which, so it might as well use (or copy) the hardcoded YUY2 one. (Except the RGB8 check in init_video_mt, but I can just switch that to check something else.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49351

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:46:01 2023 +0000, Zebediah Figura wrote: > Wait, so is the data here generated with ffmpeg or GStreamer? Both. ffmpeg to generate a black video, then GStreamer to demux it and emit only the video elementary stream. Guess I should rewrite it to GStreamer only. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49350

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:48 2023 +0000, Zebediah Figura wrote: > That shouldn't be necessary. If the sink is disconnected the source > won't even be exposed. No, the output pin always exists, you just can't connect it. I checked. <https://gitlab.winehq.org/wine/wine/-/blob/427a6845e25569c161f17775fda175b8…> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49349

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:52 2023 +0000, Zebediah Figura wrote: > We have amt_from_wg_format(), that should be usable here. Good point. It fills in a few fields in unexpected ways, but most are correct, so it's an improvement. And some of the incorrect fields look like omissions in that function, which are better fixed there. I tried running some tests locally, to check if something depends on the previous definition, but one failed because I compiled Wine without SChannel, another hit an assertion because I compiled without Vulkan, and several of them create windows all over which steal focus and make it hard to do anything else on the computer. I'll just leave that check for the CI. (And hope I don't miss it between the flaky tests already in master.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49348

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:47 2023 +0000, Zebediah Figura wrote: > That doesn't seem right. What is this doing? It silences these warnings from the Wagamama demo: ``` (wine:1578450): GStreamer-CRITICAL **: 01:44:15.602: gst_event_new_qos: assertion 'diff >= 0 || -diff <= timestamp' failed 0:00:00.608062458 1578450 0x7f9190049ef0 ERROR WINE dlls/winegstreamer/wg_parser.c:486:wg_parser_stream_notify_qos: Failed to create QOS event. (wine:1578450): GStreamer-CRITICAL **: 01:44:15.602: gst_pad_push_event: assertion 'GST_IS_EVENT (event)' failed ``` But like the format change thing, it doesn't seem to do any actual harm. And I agree it is a weird solution. I guess it's safest to remove it and leave it for later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49347

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:56 2023 +0000, Zebediah Figura wrote: > Does the application actually use IAMStreamSelect::Info()? If so we > should have tests for it. Also, ideally this'd be a separate patch. > If not I'd just drop this hunk. Does "it calls the function, but doesn't really do anything with the result" count? https://github.com/krkrz/krkr2/blob/dec49af97e174d31059c3ccd7efc700ba3c6b78… It would be satisfied if IStreamSelect::Count was a semi-stub that just returns zero, but I suspect that's not the best idea. But tests, yes, absolutely. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49346

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Alfred Agrell (＠Alcaro) Oct. 19, 2023

Oct. 19, 2023

On Mon Oct 16 23:45:46 2023 +0000, Zebediah Figura wrote: > Isn't this redundant with the same logic done in wg_parser_stream_seek()? Only if wg_parser_stream_seek has access to the duration. Which, uh, it does, so let's just force push that and pretend it didn't happen. (On a totally related note, GST_ERROR/etc shouldn't end with \n. I'll go clean that up too.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_49345

1 0