October 2023 - Wine-gitlab - list.winehq.org

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 22:21:57 2023 +0000, Zebediah Figura wrote: > 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? Worth checking, but the answer is they're the same. https://testbot.winehq.org/JobDetails.pl?Key=138797&f101=exe64.report (First - the one already acquired. Second - after changing and sleeping. Third - after decommitting and recommitting.) As I may have said, I have a vague suspicion this component is haunted. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3970#note_48919

1 0

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

16 Oct '23

On Tue Oct 3 09:21:16 2023 +0000, Rémi Bernon wrote: > I don't know whether this is correct or not, I think it'd be better to > leave this aspect aside for now as like you said this is going a bit out > of the scope of this MR. I would expect quartz to do one of three things, although others are possible: * ignore format changes entirely, * report the new format in the sample itself, via IMediaSample::GetMediaType(), * reconnect the downstream pin. In this case, as I understand, there's no real format change, it's just that GStreamer gains a bit of information more asynchronously; in that case nothing should happen. Probably we shouldn't even be reporting a format change if the translated wg_format doesn't change. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48917

1 0

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

16 Oct '23

Hi, really sorry about the very late review on this one (I was out for a while, and then spent a lot of time catching up on review while I was out, and then trying to get some more important Wine work done...) 2/7, seeking parts aside, is broadly doing the right thing (from my perspective) but I would like to see it arranged differently. What I would like to see is something like the following, in a series of individual patches: * Introduce a wg_parser_create option to use the encoded format; this should end up being translated to "chain_decodebins" basically as you have it. I would also rename that to something like "output_compressed" (with inverted meaning). Note that in this case autoplug-continue already does what we want, you shouldn't need to set the "caps" property on top of that. * Use this property, plus WG_PARSER_DECODEBIN, in the existing frontend instead of WG_PARSER_MPEGAUDIOPARSE. Delete WG_PARSER_MPEGAUDIOPARSE. * Ideally patch 1/7 should be moved from the beginning of the series to here. * Add MPEG-1 system stream support to the quartz frontend. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48918

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: > + HRESULT hr = CoCreateInstance(&CLSID_CMpegVideoCodec, NULL, CLSCTX_INPROC_SERVER, > + &IID_IBaseFilter, (void **)&filter); > + ok(hr == S_OK, "Got hr %#lx.\n", hr); > + return filter; > +} > + > +static inline BOOL compare_media_types(const AM_MEDIA_TYPE *a, const AM_MEDIA_TYPE *b) > +{ > + return !memcmp(a, b, offsetof(AM_MEDIA_TYPE, pbFormat)) > + && !memcmp(a->pbFormat, b->pbFormat, a->cbFormat); > +} > + > +enum video_type_t { > + vt_yv12, vt_y41p, vt_yuy2, vt_uyvy, > + vt_rgb24, vt_rgb32, vt_rgb565, vt_rgb555, vt_rgb8, > +}; Usually enum values are capitalized; also, _t is usually reserved for typedefs. That said, do we even want this enum? Is there any instance where we shouldn't just iterate over the whole array? Two of these formats are todo, but I think something like "todo_wine_if (i == 1 || i == 8)" would be fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3938#note_48916

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