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

[Michael Bradshaw]

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?


> > +        break;
> >      case AV_PIX_FMT_GRAY8:
> >      case AV_PIX_FMT_YUV410P:
> >      case AV_PIX_FMT_YUV411P:
> > @@ -378,7 +403,6 @@ static int libopenjpeg_encode_frame(AVCodecContext
> *avctx, AVPacket *pkt,
> >      case AV_PIX_FMT_YUV444P:
> >      case AV_PIX_FMT_YUVA420P:
> >      case AV_PIX_FMT_YUVA422P:
> > -    case AV_PIX_FMT_YUVA444P:
>
> stray change.
>

Good catch, thanks. Fixed.


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

Thanks,

Michael
-------------- 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: 2873 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130123/ef37fd8a/attachment.obj>