[FFmpeg-devel] [PATCH] libopenjpegdec: add pix fmt gbrp (9-14 bit)

[Michael Bradshaw]

On Thu, Jan 24, 2013 at 3:14 AM, Paul B Mahol <onemda at gmail.com> wrote:

> On 1/24/13, Michael Bradshaw <mjbshaw at gmail.com> wrote:
> > On Wed, Jan 23, 2013 at 11:01 PM, Michael Cinquin
> > <mc at michaelcinquin.com>wrote:
> >
> >> On 23 janv. 2013, at 22:05, Michael Bradshaw wrote:
> >>
> >> > On Wed, Jan 23, 2013 at 2:05 PM, Michael Bradshaw <mjbshaw at gmail.com>
> >> wrote:
> >> >
> >> >> Added support for decoding gbrp (9-14 bit) images. I didn't add 8 and
> >> >> 16
> >> >> bit because these should just be decoded as RGB 8 and 16 bit.
> >> >>
> >> >> Note that this hasn't been tested. I don't have any samples, and I
> >> >> can't
> >> >> figure out how to get image_to_j2k to output 9-14 bit rgb samples
> (and
> >> >> I
> >> >> couldn't generate any with FFmpeg since there's no rgb48 -> gbr
> >> converter).
> >>
> >> Here is a j2c in xyz colorspace :
> >> www.michaelcinquin.com/get/12bpc_xyz.j2c
> >> , and another j2c in rgb colorspace, both of them 12 bpc.
> >> www.michaelcinquin.com/get/12bpc_rgb.j2c
> >
> >
> > Thanks for the samples!
> >
> > To devs: this patch may not even be needed, actually (and after testing,
> it
> > doesn't even work anyway). It looks like rgb48 pix fmt is selected and
> > avctx->bits_per_raw_sample is properly set to 12 bpc (tested with the
> > 12bpc_rgb.j2c sample).
> >
>
> You have no single clue what you are doing.
>

You devs know FFmpeg far better than I do, which is why I appreciate your
feedback and reviews of patches I send in. I don't pretend to be an expert.


> Did you ever notice that RGB48 case is pure hack
> because you manully deinterleave/planarize RGB48.
>

I assume you're talking about the
libopenjpeg_copy_to_packed8/libopenjpeg_copy_to_packed16 functions in the
decoder, right? A) that code has roots that go back to before I ever
touched FFmpeg[1], and those functions weren't even written by me[2][3], so
I'm not sure what that has to do with me, and B) while the GBRP formats
were added[4] a few days after my first FFmpeg patch (despite GBR24P
existing before[5]), the decoder still needed to support RGB because it
would've been completely useless without it and without swscale's support
for the GBRP formats. But yes, I have noticed and felt this.


> Obviously you do not care for performance.


I care about performance, but I'll be honest I care more about whether or
not something is useful.


[1]
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/libopenjpeg.c;h=42c71e361cec0688b05d7b081e0dfcf19c9a1af6;hb=d50122eafaa0cc70d6186c3c106c47ed0b3490d9#l180

[2]
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=eb511ef6363af8df6f842b7f5dc37fadbc15c8dd
[3]
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=0d4a77472ad8a4d194e93379a992f66c33fbf144
[4]
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=bd97b2e1ce713a4886d909f71b6f1f5403446f53#patch2
[5]
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavutil/pixfmt.h;hb=a72580fc9efeb674257b4754ca5de0fa34b09be8#l160