Re: [PATCH] wincodecs: Trigger conversion to target format in WriteSource()

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> - if (!IsEqualGUID(&src_format, format))
> - {
> -
>    /* FIXME: should use WICConvertBitmapSource to convert */
> -
>    ERR("format %s unsupported\n", debugstr_guid(&src_format));
> -
>    return E_FAIL;
> -
>    hr = IWICBitmapFrameEncode_SetPixelFormat(iface, &src_format);
> -
>    if (FAILED(hr)) return hr;
> }

Despite the removing a questionable FIXME this part of the code won't do
what you think it does. Besides, since there is no a single todo_wine
removed with this patch adding a bunch of tests for different encoders
is needed to show that the automatic conversion is really supposed to
happen, and that it doesn't depend on the implementation detail in some
of the encoders. According to my testing not every encoder does this
kind of thing, especially when a conversion needs a palette.

[PATCH] wincodecs: Trigger conversion to target format in WriteSource()

[Vincent Povirk]

> Despite the removing a questionable FIXME this part of the code won't do
> what you think it does.

I think it will request a specific pixel format, and the encoder may
actually set a different format if it does not support that one. Am I
missing something?
> Besides, since there is no a single todo_wine
> removed with this patch adding a bunch of tests for different encoders
> is needed to show that the automatic conversion is really supposed to
> happen, and that it doesn't depend on the implementation detail in some
> of the encoders. According to my testing not every encoder does this
> kind of thing, especially when a conversion needs a palette.

As long as some of them do, it makes sense to have it in common code,
and if we need to refine the behavior for individual encoders we can
do that later.

[PATCH] wincodecs: Trigger conversion to target format in WriteSource()

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> > Despite the removing a questionable FIXME this part of the code won't do
> > what you think it does.
>
> I think it will request a specific pixel format, and the encoder may
> actually set a different format if it does not support that one. Am I
> missing something?

That's correct.
> > Besides, since there is no a single todo_wine
> > removed with this patch adding a bunch of tests for different encoders
> > is needed to show that the automatic conversion is really supposed to
> > happen, and that it doesn't depend on the implementation detail in some
> > of the encoders. According to my testing not every encoder does this
> > kind of thing, especially when a conversion needs a palette.
>
> As long as some of them do, it makes sense to have it in common code,
> and if we need to refine the behavior for individual encoders we can
> do that later.

First thing to do (as usually) is to write some tests to see that auto-
conversion actually is supposed to happen. Most likely In reality a target
encoder simply needs to be fixed in order to support the missing format
natively.