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

[Michael Niedermayer]

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 &&

> >> +    case 2: match = match && descriptor.comp[1].depth_minus1 + 1 == image->comps[1].prec &&
...
> >> +    case 1: match = match && descriptor.comp[0].depth_minus1 + 1 == image->comps[0].prec;

                                                                                              ^^^
> >> +                             1 == image->comps[0].dx &&
> >> +                             1 == image->comps[0].dy;

i dont think these lines have any effect on what ends in match


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120503/e340d682/attachment.asc>