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

[Michael Niedermayer]

On Thu, May 03, 2012 at 02:19:24PM -0600, Michael Bradshaw wrote:
> 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

merged

thanks

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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/a38986e2/attachment.asc>