2/14: ``` + if (FAILED(hr = CoCreateInstance( + &CLSID_MemoryAllocator, NULL, CLSCTX_INPROC_SERVER, &IID_IMemAllocator, (void **)&allocator))) + return hr; ``` Eh, what's up with the indentation here? 6/14: ``` static HRESULT color_sink_connect(struct strmbase_sink *iface, IPin *peer, const AM_MEDIA_TYPE *mt) { + if (!get_subtype(mt)) + { + TRACE("Connection refused\n"); + return VFW_E_TYPE_NOT_ACCEPTED; + } + + /* TODO: Set up color conversion */ FIXME("stub\n"); - return VFW_E_TYPE_NOT_ACCEPTED; + return S_OK; } ``` You don't need the media type check; strmbase calls query_accept() before reaching this callback. Since you're not doing anything else either, you can just leave out the callback entirely. 9/14: ``` + header = &((VIDEOINFOHEADER *)iface->pin.mt.pbFormat)->bmiHeader; ``` Generally you want to check formattype before doing this, especially since FORMAT_VideoInfo2 is a thing. 10/14: ``` + video_info = calloc(1, sizeof(*video_info)); + memcpy(video_info, output_mt->pbFormat, sizeof(*video_info)); + video_info->bmiHeader.biWidth = input_video_info->bmiHeader.biWidth; + video_info->bmiHeader.biHeight = input_video_info->bmiHeader.biHeight; + video_info->bmiHeader.biSizeImage = abs(calculate_stride(&video_info->bmiHeader)) * video_info->bmiHeader.biHeight; + + memcpy(dmo_mt, output_mt, sizeof(*dmo_mt)); + dmo_mt->pbFormat = (BYTE *)video_info; + dmo_mt->lSampleSize = video_info->bmiHeader.biSizeImage; ``` Why do you need to do these? Does the converter ignore width/height mismatch? I don't see tests for that, unless I'm blind. (Also, that biSizeImage calculation is rather awkward. I think it would make more sense to just multiply width * abs(height) * depth.) Also, I'd recommend adding a FIXME for nontrivial rcSource/rcTarget handling. I've seen applications depend on that. 11/14: ``` -IMPORTS = strmiids dxguid strmbase uuid dsound msacm32 msvfw32 ole32 oleaut32 rpcrt4 user32 gdi32 advapi32 winmm msvcrt dmoguids wmcodecdspuuid +IMPORTS = mfplat strmiids dxguid strmbase uuid dsound msacm32 msvfw32 ole32 oleaut32 rpcrt4 user32 gdi32 advapi32 winmm msvcrt dmoguids wmcodecdspuuid ``` mfplat should be a delay-import. I'm not convinced you really need it, though. The only thing you're using it for is MFCopyImage(), and this is probably something we should be avoiding in the first place; this should be zero-copy if at all possible. ``` + output_image_size = header->biWidth * header->biHeight * (subtype->bitcount / 8); ``` Not header->biSizeImage? Also I don't think that handles negative biHeight. ``` + if (IMediaSample_IsPreroll(src_sample) == S_OK) + flags |= ICDECOMPRESS_PREROLL; ``` That looks left over, as does the other VFW flag code. ``` + hr = IMediaSample_GetTime(src_sample, &start, &stop); + + /* perform color conversion */ + src_buffer = create_buffer_for_sample(src_sample); + hr = IMediaObject_ProcessInput(filter->dmo, 0, &src_buffer->IMediaBuffer_iface, + hr == S_OK ? DMO_INPUT_DATA_BUFFERF_TIME & DMO_INPUT_DATA_BUFFERF_TIMELENGTH : 0, start, stop - start); ``` That's hard to read, doesn't correctly handle VFW_S_NO_STOP_TIME, and looks like you typed & when you meant |. ``` + if (hr == S_OK) + IMediaSample_SetTime(dst_sample, &start, &stop); + else if (hr == VFW_S_NO_STOP_TIME) + IMediaSample_SetTime(dst_sample, &start, NULL); + else + IMediaSample_SetTime(dst_sample, NULL, NULL); ``` You've overwritten the hr you're trying to use here. You might as well just set this earlier. ``` + if (input_bmi_header->biBitCount < output_bmi_header->biBitCount && output_bmi_header->biBitCount == 32) + { + /* Fix the value of the alpha channel. DMO uses 0xff, whilst quartz uses 0x00. */ + data = (UINT32 *)dst_buffer->data; + + for (i = 0; i < input_bmi_header->biHeight * input_bmi_header->biWidth; i++) + *data++ &= 0xffffff; + } ``` Shouldn't this check for RGB32 specifically? This would be wrong for ARGB32. Also, does this not affect ARGB32 -> RGB32? But anyway, this is really not ideal for performance, and maybe it means we shouldn't be using colorcnv. 12/14: ``` + if (memcmp(mt, &filter->source.pin.mt, offsetof(AM_MEDIA_TYPE, pbFormat)) || + memcmp(mt->pbFormat, filter->source.pin.mt.pbFormat, mt->cbFormat)) ``` || at the beginning of the line. ``` + if (FAILED(hr = IMediaObject_SetOutputType(filter->dmo, 0, &dmo_mt, 0))) + WARN("Failed to update media type, hr %#lx.\n", hr); ``` I would make this an ERR; this shouldn't happen. 13/14: ``` +static HRESULT color_sink_end_flush(struct strmbase_sink *iface) +{ + struct color_converter *filter = impl_from_strmbase_filter(iface->pin.filter); + if (filter->source.pin.peer) + return IPin_EndFlush(filter->source.pin.peer); + return S_OK; +} ``` strmbase should already do this for you. 14/14: ``` +HRESULT color_sink_can_block(struct strmbase_sink *iface) ``` Missing static. Also, I would name it consistently with the method name, color_sink_receive_can_block(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11679#note_149712