- 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/quartz/tests/mpegsplit.c: > HRESULT hr; > > hr = IMediaSample_GetTime(sample, &start, &end); > - todo_wine_if (hr == VFW_S_NO_STOP_TIME) ok(hr == S_OK, "Got hr %#lx.\n", hr); > + todo_wine_if (hr == VFW_S_NO_STOP_TIME) { > + ok(hr == S_OK || (filter->sample_count > 0 && hr == VFW_E_SAMPLE_TIME_NOT_SET), "Got hr %#lx.\n", hr); > + } 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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48910

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/quartz/tests/mpegsplit.c: > ok(ret, "Failed to delete file, error %lu.\n", GetLastError()); > } > > +static void test_video_file(void) > +{ > + const WCHAR *filename = load_resource(L"test.mpg"); > + IBaseFilter *filter = create_mpeg_splitter(); > + struct testfilter testsink_video; > + struct testfilter testsink_audio; > + IPin *source_video = NULL; > + IPin *source_audio = NULL; > + IMediaControl *control; > + IFilterGraph2 *graph; > + IAMStreamSelect* sel; > + DWORD n_streams; "stream_count" is the usual naming convention for this kind of thing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48911

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

2025

2024

2023

2022

wine-gitlab