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

[Michael Bradshaw]

On Thu, May 3, 2012 at 10:05 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, May 03, 2012 at 06:57:34AM -0600, Michael Bradshaw 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
>
>
> Sorry for the slow awnser, i refered to the ; that possibly was meant
> to be a &&

Oh wow, I totally missed that one. Yes, it was supposed to be a &&.
Thanks. Attached patch fixes that, as well as left-shifting
log2_chroma instead of adding it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libopenjpegdec-respect-JP2-color-space-fix-ticket-11.patch
Type: application/octet-stream
Size: 7103 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120503/c14ecba4/attachment.obj>