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

[Michael Bradshaw]

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).