October 2023 - Wine-gitlab - list.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: > +}; > + > +HRESULT mpeg_video_codec_create(IUnknown *outer, IUnknown **out) > +{ > + static const struct wg_format output_format = > + { > + .major_type = WG_MAJOR_TYPE_VIDEO, > + .u.video = { > + .format = WG_VIDEO_FORMAT_I420, > + /* size doesn't matter, this one is only used to check if the GStreamer plugin exists */ > + }, > + }; > + static const struct wg_format input_format = > + { > + .major_type = WG_MAJOR_TYPE_VIDEO_MPEG1, > + .u.video_mpeg1 = {}, You can delete this line. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48907

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: > IUnknown **object, IUnknown **unknown) > { > - FIXME("iface %p, index %ld, mt %p, flags %p, lcid %p, group %p, name %p, object %p, unknown %p, stub!\n", > - iface, index, mt, flags, lcid, group, name, object, unknown); > - return E_NOTIMPL; > + struct parser *filter = impl_from_IAMStreamSelect(iface); > + wg_parser_stream_t stream = wg_parser_get_stream(filter->wg_parser, index); > + > + FIXME("filter %p, index %ld, mt %p, flags %p, lcid %p, group %p, name %p, object %p, unknown %p, semi-stub!\n", > + filter, index, mt, flags, lcid, group, name, object, unknown); > + > + if (!filter->sink.pin.peer) > + return VFW_E_NOT_CONNECTED; > + if (index < 0 || index >= wg_parser_get_stream_count(filter->wg_parser)) > + return S_FALSE; > + 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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48908

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: > > static HRESULT WINAPI stream_select_Count(IAMStreamSelect *iface, DWORD *count) > { > - FIXME("iface %p, count %p, stub!\n", iface, count); > - return E_NOTIMPL; > + struct parser *filter = impl_from_IAMStreamSelect(iface); > + TRACE("filter %p, count %p\n", filter, count); > + if (filter->sink.pin.peer) > + *count = wg_parser_get_stream_count(filter->wg_parser); > + else > + *count = 0; > + return S_OK; This probably needs to take the filter CS. Same for Info() for that matter. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48909

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