- wine-gitlab - winehq.org

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/quartz_transform.c: > + } static const video_types[] = { > + { &MEDIASUBTYPE_YV12, WG_VIDEO_FORMAT_YV12, 12, MAKEFOURCC('Y','V','1','2') }, > + { &MEDIASUBTYPE_YUY2, WG_VIDEO_FORMAT_YUY2, 16, MAKEFOURCC('Y','U','Y','2') }, > + { &MEDIASUBTYPE_UYVY, WG_VIDEO_FORMAT_UYVY, 16, MAKEFOURCC('U','Y','V','Y') }, > + { &MEDIASUBTYPE_RGB24, WG_VIDEO_FORMAT_BGR, 24, BI_RGB }, > + { &MEDIASUBTYPE_RGB32, WG_VIDEO_FORMAT_BGRx, 32, BI_RGB }, > + { &MEDIASUBTYPE_RGB565, WG_VIDEO_FORMAT_RGB16, 16, BI_BITFIELDS }, > + { &MEDIASUBTYPE_RGB555, WG_VIDEO_FORMAT_RGB15, 16, BI_RGB }, > + }; > + > + MPEG1VIDEOINFO *input_format; > + VIDEOINFO *output_format; > + > + if (!filter->sink.pin.peer) > + return VFW_S_NO_MORE_ITEMS; > + That shouldn't be necessary. If the sink is disconnected the source won't even be exposed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48903

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/quartz_transform.c: > +static HRESULT mpeg_video_codec_source_get_media_type(struct transform *filter, unsigned int index, AM_MEDIA_TYPE *mt) > +{ > + struct { > + const GUID *subtype; > + wg_video_format wg_format; > + int bits_per_pixel; > + DWORD compression; > + } static const video_types[] = { > + { &MEDIASUBTYPE_YV12, WG_VIDEO_FORMAT_YV12, 12, MAKEFOURCC('Y','V','1','2') }, > + { &MEDIASUBTYPE_YUY2, WG_VIDEO_FORMAT_YUY2, 16, MAKEFOURCC('Y','U','Y','2') }, > + { &MEDIASUBTYPE_UYVY, WG_VIDEO_FORMAT_UYVY, 16, MAKEFOURCC('U','Y','V','Y') }, > + { &MEDIASUBTYPE_RGB24, WG_VIDEO_FORMAT_BGR, 24, BI_RGB }, > + { &MEDIASUBTYPE_RGB32, WG_VIDEO_FORMAT_BGRx, 32, BI_RGB }, > + { &MEDIASUBTYPE_RGB565, WG_VIDEO_FORMAT_RGB16, 16, BI_BITFIELDS }, > + { &MEDIASUBTYPE_RGB555, WG_VIDEO_FORMAT_RGB15, 16, BI_RGB }, > + }; Let's match the coding style used elsewhere please, regarding things like spacing and storage classifiers placement; see e.g. amt_to_wg_format_video for an example. "wg_video_format" should be "enum wg_video_format". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48904

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_parser.c: > * file (or other medium), but gst_event_new_qos() expects the timestamp in > * running time. */ > stream_time = gst_segment_to_running_time(&stream->segment, GST_FORMAT_TIME, params->timestamp * 100); > - if (stream_time == -1) > + if (stream_time == -1 || -params->diff*100 >= stream_time) That doesn't seem right. What is this doing? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48900

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_format.c: > return NULL; > > gst_video_info_set_format(&info, video_format, format->u.video.width, abs(format->u.video.height)); > + info.fps_n = format->u.video.fps_n; > + info.fps_d = format->u.video.fps_d; > + if (!format->u.video.fps_d && !format->u.video.fps_n) > + info.fps_d = 1; This may be correct (although I'm curious if it actually matters here?) but should be a separate patch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48901

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/quartz_parser.c: > if (seeking->llStop) > stop_flags = AM_SEEKING_AbsolutePositioning; > wg_parser_stream_seek(filter->sources[0]->wg_stream, seeking->dRate, > - seeking->llCurrent, seeking->llStop, AM_SEEKING_AbsolutePositioning, stop_flags); > + seeking->llCurrent, seeking->llStop == seeking->llDuration ? -1 : seeking->llStop, > + AM_SEEKING_AbsolutePositioning, stop_flags); Isn't this redundant with the same logic done in wg_parser_stream_seek()? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48898

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_parser.c: > stop_type = GST_SEEK_TYPE_NONE; > > if (!push_event(get_stream(params->stream)->my_sink, gst_event_new_seek(params->rate, GST_FORMAT_TIME, > - flags, start_type, params->start_pos * 100, stop_type, params->stop_pos * 100))) > + flags, start_type, params->start_pos * 100, stop_type, > + params->stop_pos == -1 ? -1 : params->stop_pos * 100))) I think we want this, but this isn't relevant to this patch and should be split. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48899

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/quartz_parser.c: > * but as long as every sample fits into our allocator, we're fine. */ > return format->u.video_cinepak.width * format->u.video_cinepak.height * 3; > > + case WG_MAJOR_TYPE_VIDEO_MPEG1: > + return wg_format_get_bytes_for_uncompressed(WG_VIDEO_FORMAT_YV12, > + format->u.video_mpeg1.width, format->u.video_mpeg1.height); > + This is overestimation because we can't do any better, right? In that case it probably deserves a comment just to clarify that's what's happening. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48896

1 0

Re: [PATCH v13 0/7] MR3938: winegstreamer: Implement CLSID_CMpegVideoCodec
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

Zebediah Figura (@zfigura) commented about dlls/winegstreamer/wg_format.c: > if (gst_video_info_from_caps(&info, caps)) > wg_format_from_video_info(format, &info); > } > - else if (!strcmp(name, "audio/mpeg")) > + else if (!strcmp(name, "audio/mpeg") && gst_structure_get_boolean(structure, "parsed", &parsed) && parsed) This is correct, I think, but also orthogonal to the purpose of this patch. Can you please split this out into a separate patch? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48897

1 0

Re: [PATCH v6 0/1] MR3970: quartz/tests: Improve VMR9 ChangeD3DDevice test
by Zebediah Figura (＠zfigura) 16 Oct '23

16 Oct '23

On Mon Oct 16 21:55:32 2023 +0000, Alfred Agrell wrote: > Unfortunately, we do. I had sleeps everywhere during development, then > removed every one I could, but that specific one is load-bearing. > https://testbot.winehq.org/JobDetails.pl?Key=138782&f101=exe64.report > https://testbot.winehq.org/JobDetails.pl?Key=138783&f101=exe64.report > I tried swapping it for IBaseFilter_GetState(), but that just returned > immediately then gave the same segfault. https://testbot.winehq.org/JobDetails.pl?Key=138786&f101=exe64.report > Calling GetState twice worked, but I'm pretty sure that's just because > those calls take long enough to satisfy its timing demands. A sleep is > cleaner. (Slightly. Still ugly.) > I could also move the sleep, but I don't think that would accomplish anything. Hrm, that's suspicious, IMemAllocator::GetBuffer() returns VFW_E_NOT_COMMITTED. Does the VMR's IMemAllocator change by any chance when we reconnect? I.e. is IMemInputPin::GetAllocator() actually returning the same allocator that it was returning before? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3970#note_48893

1 0

Re: [PATCH v6 0/1] MR3970: quartz/tests: Improve VMR9 ChangeD3DDevice test
by Alfred Agrell (＠Alcaro) 16 Oct '23

16 Oct '23

On Mon Oct 16 21:25:57 2023 +0000, Zebediah Figura wrote: > Do we still need this? Unfortunately, we do. I had sleeps everywhere during development, then removed every one I could, but that specific one is load-bearing. https://testbot.winehq.org/JobDetails.pl?Key=138782&f101=exe64.report https://testbot.winehq.org/JobDetails.pl?Key=138783&f101=exe64.report I tried swapping it for IBaseFilter_GetState(), but that just returned immediately then gave the same segfault. https://testbot.winehq.org/JobDetails.pl?Key=138786&f101=exe64.report Calling GetState twice worked, but I'm pretty sure that's just because those calls take long enough to satisfy its timing demands. A sleep is cleaner. (Slightly. Still ugly.) I could also move the sleep, but I don't think that would accomplish anything. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3970#note_48883

1 0

2025

2024

2023

2022

wine-gitlab