- 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: > + return VFW_S_NO_MORE_ITEMS; > + > + input_format = (MPEG1VIDEOINFO*)filter->sink.pin.mt.pbFormat; > + > + output_format = CoTaskMemAlloc(sizeof(*output_format)); > + if (!output_format) > + return E_OUTOFMEMORY; > + > + memcpy(output_format, &input_format->hdr, sizeof(VIDEOINFOHEADER)); > + output_format->bmiHeader.biPlanes = 1; > + output_format->bmiHeader.biBitCount = video_types[index].bits_per_pixel; > + output_format->bmiHeader.biCompression = video_types[index].compression; > + output_format->bmiHeader.biSizeImage = > + wg_format_get_bytes_for_uncompressed(video_types[index].wg_format, > + output_format->bmiHeader.biWidth, output_format->bmiHeader.biHeight); > + output_format->dwBitRate = MulDiv(output_format->bmiHeader.biSizeImage * 8, 10000000, output_format->AvgTimePerFrame); We have amt_from_wg_format(), that should be usable here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48906

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: > + 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 }, > + }; > + > + MPEG1VIDEOINFO *input_format; const MPEG1VIDEOINFO * -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48905

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: > + { &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; > + > + if (index >= 7) > + return VFW_S_NO_MORE_ITEMS; ARRAY_SIZE(video_types) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48902

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 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

2025

2024

2023

2022

wine-gitlab