October 2023 - wine-gitlab

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/mpegvideo.c: > + IPin_Release(pin); > + > + ref = IBaseFilter_Release(filter); > + ok(!ref, "Got outstanding refcount %ld.\n", ref); > +} > + > +struct testfilter > +{ > + struct strmbase_filter filter; > + struct strmbase_source source; > + struct strmbase_sink sink; > + const AM_MEDIA_TYPE *mt; > + unsigned int got_sample, got_new_segment, got_eos, got_begin_flush, got_end_flush; > + REFERENCE_TIME expected_start_time; > + REFERENCE_TIME expected_stop_time; > + BOOL wine_wrong_stop_time; The usual convention for something like this would be "todo_stop_time". Same for "wine_fail" below, that would be "todo". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48915

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/mpegvideo.c: > +static HRESULT WINAPI testsink_Receive(struct strmbase_sink *iface, IMediaSample *sample) > +{ > + struct testfilter *filter = impl_from_strmbase_filter(iface->pin.filter); > + REFERENCE_TIME start, stop; > + HRESULT hr; > + LONG size; > + > + size = IMediaSample_GetSize(sample); > + ok(size == 1536, "Got size %lu.\n", size); > + size = IMediaSample_GetActualDataLength(sample); > + ok(size == 1536, "Got actual size %lu.\n", size); > + > + start = 0xdeadbeef; > + stop = 0xdeadbeef; > + hr = IMediaSample_GetTime(sample, &start, &stop); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); Can you please also check (at least the return value of) IMediaSample::GetMediaType()? I'm rather curious if it's updated or not. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48914

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/mpegvideo.c: > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + > + size = IMediaSample_GetSize(sample); > + ok(size == 256, "Got size %ld.\n", size); > + > + hr = IMediaSample_SetTime(sample, NULL, NULL); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + > + sink->expected_start_time = 0; > + sink->expected_stop_time = 0; > + sink->wine_wrong_stop_time = FALSE; > + hr = IMediaSample_SetTime(sample, &sink->expected_start_time, &sink->expected_stop_time); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + > + test_send_video(input, sample); > + ok(sink->got_sample >= 1, "Got %u calls to Receive().\n", sink->got_sample); Shouldn't we get an exact number of frames here? Or is that not consistent somehow? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48913

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/mpegvideo.c: > + > + memcpy(target_data, data, len); > + hr = IMediaSample_SetActualDataLength(sample, len); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + > + hr = IMemInputPin_Receive(input, sample); > + todo_wine_if(wine_fail) /* 0xc00d6d61 is MF_E_TRANSFORM_STREAM_CHANGE */ > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > +} > + > +static void test_send_video(IMemInputPin *input, IMediaSample *sample) > +{ > + /* ffmpeg -t 1.0 -s 32x24 -f rawvideo -pix_fmt rgb24 -r 25 -i /dev/zero -c:v mpeg1video -q:v 0 -format mpegvideo empty.mpeg */ > + /* gst-launch-1.0 filesrc location=empty.mpeg ! mpegpsdemux ! mpegvideoparse ! filesink location=empty-es.mpeg */ > + /* then truncate to taste */ > + /* each 00 00 01 b3 or 00 00 01 00 starts a new frame, except the first 00 00 01 00 after a 00 00 01 b3 */ Wait, so is the data here generated with ffmpeg or GStreamer? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48912

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

2025

2024

2023

2022

wine-gitlab October 2023