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

[Michael Bradshaw]

On Thu, May 3, 2012 at 1:57 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, May 03, 2012 at 12:30:00PM -0600, Michael Bradshaw wrote:
>> 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.
>
>>  libopenjpegdec.c |  135 ++++++++++++++++++++++++++++++-------------------------
>>  1 file changed, 74 insertions(+), 61 deletions(-)
>> ec2e04707d23e088edc3c98f3a2a51dc30e1d311  0001-libopenjpegdec-respect-JP2-color-space-fix-ticket-11.patch
>> From 74035d5a1e8a91258071986e5699cd86369916d4 Mon Sep 17 00:00:00 2001
>> From: Michael Bradshaw <mbradshaw at sorensonmedia.com>
>> Date: Thu, 3 May 2012 12:27:04 -0600
>> Subject: [PATCH] libopenjpegdec: respect JP2 color space, fix ticket 1179
>
> should be ok if it has been tested with a representative set of jp2
> images

Tested with yuv444p, yuv440p, yuv420p, gray, rgb24, rgb48, rgba,
yuv444p9, yuv444p10, yuv444p16, yuv422p16, yuv420p9 JP2 images and
yuv420p9, yuv444p9, yuv444p10, yuv422p10, rgba, rgb24, rgb48, and gray
J2K images. Works with all of these. I've pushed it to my repo:

git://github.com/mjbshaw/FFmpeg-OpenJPEG-J2K-Encoder.git

Thanks,

Michael