[FFmpeg-devel] [PATCH] libopenjpegenc: add 9-15 bit RGB output

[Michael Bradshaw]

On Wed, Jan 23, 2013 at 12:20 PM, Michael Bradshaw <mjbshaw at gmail.com>wrote:

> On Wed, Jan 23, 2013 at 11:11 AM, Paul B Mahol <onemda at gmail.com> wrote:
>
>> On 1/23/13, Michael Bradshaw <mjbshaw at gmail.com> wrote:
>> > On Wed, Jan 23, 2013 at 6:53 AM, Paul B Mahol <onemda at gmail.com> wrote:
>> >
>> >> Patch is silly. It adds option to shift pixel values by some random
>> >> unlimited number.
>> >>
>> >
>> > It's not unlimited. The range [0, 7] is specified.
>> >
>> >
>> >> Even feature is far from being useful at all and it is not documented
>> at
>> >> all.
>> >>
>> >
>> > I felt naming and documenting awkward for this feature, so I left it
>> undone
>> > (rather than done poorly). Suggestions are welcome.
>> >
>> >
>> >> IMHO it is much better to support planar RGB stuff, you just need to
>> >> copy plane data
>> >> in right order. (Instead of manually deinterleaving (slow) packed RGB).
>> >>
>> >> So you should add support for planar rgb pixel formats in
>> >> decoder/encoder instead.
>> >
>> >
>> > Alright, attached is a version that does that (needs reviewing). Now
>> people
>> > can decide which they prefer. One downside is that it doesn't support
>> > alpha, and there aren't any conversion in swscale (as far as I know) to
>> > allow people to go rgb48 -> bgrp10 etc. But it does seems like a more
>> > "proper" solution.
>>
>> It is completly legit to support this pix fmts. So this patch should not
>> depend
>> on questionable acceptability of previous patch.
>> And conversion can be added at any time.
>
>
> Sounds good to me. I guess I should've made it a different email/patch
> submission.
>
>
>> > @@ -369,6 +376,24 @@ static int libopenjpeg_encode_frame(AVCodecContext
>> *avctx, AVPacket *pkt,
>> >      case AV_PIX_FMT_RGBA64:
>> >          cpyresult = libopenjpeg_copy_packed16(avctx, frame, image);
>> >          break;
>> > +    case AV_PIX_FMT_GBR24P:
>> > +        bgrframe = *frame;
>> > +        bgrframe.data[0] = frame->data[2]; // swap to be rgb
>> > +        bgrframe.data[1] = frame->data[0];
>> > +        bgrframe.data[2] = frame->data[1];
>> > +        cpyresult = libopenjpeg_copy_unpacked8(avctx, &bgrframe,
>> image);
>> > +        break;
>> > +    case AV_PIX_FMT_GBRP9:
>> > +    case AV_PIX_FMT_GBRP10:
>> > +    case AV_PIX_FMT_GBRP12:
>> > +    case AV_PIX_FMT_GBRP14:
>> > +    case AV_PIX_FMT_GBRP16:
>> > +        bgrframe = *frame;
>> > +        bgrframe.data[0] = frame->data[2]; // swap to be rgb
>> > +        bgrframe.data[1] = frame->data[0];
>> > +        bgrframe.data[2] = frame->data[1];
>> > +        cpyresult = libopenjpeg_copy_unpacked16(avctx, &bgrframe,
>> image);
>>
>> You could merge this two groups ...
>>
>
> I thought about it, but I need the function call separate because of the 8
> vs 16 bit copy. And the frame setting/pointer swapping needs to be done
> before the function call so I can't use fall-through. Or did you have
> something else in mind?
>

I just realized I'm not swapping linesizes, and to be pedantic I've updated
the patch to swap linesizes.

>          cpyresult = libopenjpeg_copy_unpacked8(avctx, frame, image);
>> >          break;
>> >      case AV_PIX_FMT_GRAY16:
>> > @@ -505,6 +529,8 @@ AVCodec ff_libopenjpeg_encoder = {
>> >      .capabilities   = 0,
>> >      .pix_fmts       = (const enum AVPixelFormat[]) {
>> >          AV_PIX_FMT_RGB24, AV_PIX_FMT_RGBA, AV_PIX_FMT_RGB48,
>> AV_PIX_FMT_RGBA64,
>> > +        AV_PIX_FMT_GBR24P,
>> > +        AV_PIX_FMT_GBRP9, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12,
>> AV_PIX_FMT_GBRP14, AV_PIX_FMT_GBRP16,
>> >          AV_PIX_FMT_GRAY8, AV_PIX_FMT_GRAY8A, AV_PIX_FMT_GRAY16,
>> >          AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUVA420P,
>> >          AV_PIX_FMT_YUV440P, AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUVA422P,
>> > --
>> > 1.7.10.2 (Apple Git-33)
>> >
>>
>> Missing decoder changes.
>
>
> I figured I'd do that in a different patch in a day or two when I have
> some time.
>

Just about to post the decoder. Got to it sooner than I thought I could.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libopenjpegenc-add-support-for-pix-fmt-gbrp-8-16-bit.patch
Type: application/octet-stream
Size: 3191 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130123/e00fd18a/attachment.obj>