[FFmpeg-devel] [PATCH] libopenjpegdec: respect JP2 color space, fix ticket 1179

[Michael Bradshaw]

On Thu, May 3, 2012 at 6:57 AM, Michael Bradshaw
<mbradshaw at sorensonmedia.com> wrote:
> On Thu, May 3, 2012 at 6:08 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> On Wed, May 02, 2012 at 12:20:25PM -0600, Michael Bradshaw wrote:
>>> Attached patch fixes ticket 1179 for me (mostly*) by respecting the
>>> JP2 color space, if it is set. It also adds support for yuv444p. It
>>> still has to guess the color space for J2K frames.
>>>
>>> Feel free to be nitpicky about the patch; for example there were times
>>> I debated between a switch and a for with an if, trying do what is
>>> consistent with the rest of ffmpeg, and it's possible I may done
>>> something inconsistent.
>>>
>>> *I say mostly because it fixes it for JP2 frames, but J2K frames still
>>> have the potential to be guessed incorrectly (i.e. yuv444p and rgb24
>>> can get mixed up). However, it is not possible, as far as I know, to
>>> correctly guess between yuv444p and rgb24 in a J2K frame without
>>> hinting from the user, so this patch fixes it as much as can be
>>> automatically guessed. I tried passing a "-pix_fmt yuv444p" as an
>>> input option to see if the user could hint the input pixel format, but
>>> ffmpeg rejected it ("Option pixel_format not found."). Should I add a
>>> "-pix_fmt" option for the decoder so the user can hint the input pixel
>>> format?
>> [...]
>>> -    switch (compRatio) {
>>> -    case 0111111: goto libopenjpeg_yuv444_rgb;
>>> -    case 0111212: return PIX_FMT_YUV440P;
>>> -    case 0112121: goto libopenjpeg_yuv422;
>>> -    case 0112222: goto libopenjpeg_yuv420;
>>> -    default: goto libopenjpeg_rgb;
>>> +    switch (descriptor.nb_components) {
>>> +    case 4: match = match && descriptor.comp[3].depth_minus1 + 1 == image->comps[3].prec &&
>>> +                             descriptor.log2_chroma_w + 1 == image->comps[3].dx &&
>>> +                             descriptor.log2_chroma_h + 1 == image->comps[3].dy;
>>> +    case 3: match = match && descriptor.comp[2].depth_minus1 + 1 == image->comps[2].prec &&
>>> +                             descriptor.log2_chroma_w + 1 == image->comps[2].dx &&
>>> +                             descriptor.log2_chroma_h + 1 == image->comps[2].dy;
>>> +    case 2: match = match && descriptor.comp[1].depth_minus1 + 1 == image->comps[1].prec &&
>>> +                             descriptor.log2_chroma_w + 1 == image->comps[1].dx &&
>>> +                             descriptor.log2_chroma_h + 1 == image->comps[1].dy;
>>> +    case 1: match = match && descriptor.comp[0].depth_minus1 + 1 == image->comps[0].prec;
>>
>>> +                             1 == image->comps[0].dx &&
>>> +                             1 == image->comps[0].dy;
>>
>> this looks a bit odd, i dont think this does what you intended it to
>> do
>
> May I ask which part you're referring to? The whole switch statement
> or just the last two lines? If you're talking about the last two
> lines, I'm trying to ensure the first component has a full size (i.e.
> for yuv420, dx/dy for first component will be 1, dx/dy of second and
> third components will be 2).

Perhaps I should be doing

1 << descriptor.log2_chroma_w
1 << descriptor.log2_chroma_h

In each case though... it's the same results for the supported pixel
formats, but this seems a little more consistent with how it's
normally handled. Thoughts?