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

[Paul B Mahol]

On 1/23/13, Michael Bradshaw <mjbshaw at gmail.com> wrote:
> On Wed, Jan 23, 2013 at 2:49 AM, Paul B Mahol <onemda at gmail.com> wrote:
>
>> On 1/23/13, Michael Bradshaw <mjbshaw at gmail.com> wrote:
>> > As per Michael Cinquin's request and Reimar's suggestion, I added right
>> > shifting 16-bit RGB by a user specified about to achieve 9-15 bit RGB
>> > output.
>> >
>> > As far as I can tell, it's working fine, but I'd really appreciate a
>> sanity
>> > check on this. Also, I'm not a big fan of the "shr" (reminiscent of
>> > IA32
>> > shift right) option name but I couldn't think of a particularly good
>> > name
>> > suggestion. Feel free to suggest a better name.
>> >
>> > Thanks,
>> >
>> > Michael
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel at ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>>
>>
>> Patch is missing, thus rejected.
>
>
> Wow. I am so sorry! Attached.
>

> From 86866fb2a911519d6c8da8f48419ffa5aa20be38 Mon Sep 17 00:00:00 2001
> From: Michael Bradshaw <mjbshaw at gmail.com>
> Date: Wed, 23 Jan 2013 06:34:52 -0700
> Subject: [PATCH] libopenjpegenc: add 9-15 bit RGB output
>
> Signed-off-by: Michael Bradshaw <mjbshaw at gmail.com>
> ---
>  libavcodec/libopenjpegenc.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
> index 13e8ef9..a9dbb62 100644
> --- a/libavcodec/libopenjpegenc.c
> +++ b/libavcodec/libopenjpegenc.c
> @@ -55,6 +55,7 @@ typedef struct {
>      int disto_alloc;
>      int fixed_alloc;
>      int fixed_quality;
> +    int shr;
>  } LibOpenJPEGContext;
>
>  static void error_callback(const char *msg, void *data)
> @@ -74,6 +75,7 @@ static void info_callback(const char *msg, void *data)
>
>  static opj_image_t *mj2_create_image(AVCodecContext *avctx, opj_cparameters_t *parameters)
>  {
> +    LibOpenJPEGContext *ctx = avctx->priv_data;
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
>      opj_image_cmptparm_t *cmptparm;
>      opj_image_t *img;
> @@ -150,8 +152,8 @@ static opj_image_t *mj2_create_image(AVCodecContext *avctx, opj_cparameters_t *p
>          return NULL;
>      }
>      for (i = 0; i < numcomps; i++) {
> -        cmptparm[i].prec = desc->comp[i].depth_minus1 + 1;
> -        cmptparm[i].bpp  = desc->comp[i].depth_minus1 + 1;
> +        cmptparm[i].prec = desc->comp[i].depth_minus1 + 1 - ctx->shr;
> +        cmptparm[i].bpp  = desc->comp[i].depth_minus1 + 1 - ctx->shr;
>          cmptparm[i].sgnd = 0;
>          cmptparm[i].dx = sub_dx[i];
>          cmptparm[i].dy = sub_dy[i];
> @@ -245,7 +247,7 @@ static int libopenjpeg_copy_packed8(AVCodecContext *avctx, const AVFrame *frame,
>      return 1;
>  }
>
> -static int libopenjpeg_copy_packed16(AVCodecContext *avctx, const AVFrame *frame, opj_image_t *image)
> +static int libopenjpeg_copy_packed16(AVCodecContext *avctx, const AVFrame *frame, opj_image_t *image, int shr)
>  {
>      int compno;
>      int x;
> @@ -267,7 +269,7 @@ static int libopenjpeg_copy_packed16(AVCodecContext *avctx, const AVFrame *frame
>              image_index = y * avctx->width;
>              frame_index = y * (frame->linesize[0] / 2) + compno;
>              for (x = 0; x < avctx->width; ++x) {
> -                image->comps[compno].data[image_index++] = frame_ptr[frame_index];
> +                image->comps[compno].data[image_index++] = frame_ptr[frame_index] >> shr;
>                  frame_index += numcomps;
>              }
>          }
> @@ -367,7 +369,7 @@ static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>          break;
>      case AV_PIX_FMT_RGB48:
>      case AV_PIX_FMT_RGBA64:
> -        cpyresult = libopenjpeg_copy_packed16(avctx, frame, image);
> +        cpyresult = libopenjpeg_copy_packed16(avctx, frame, image, ctx->shr);
>          break;
>      case AV_PIX_FMT_GRAY8:
>      case AV_PIX_FMT_YUV410P:
> @@ -484,6 +486,7 @@ static const AVOption options[] = {
>      { "disto_alloc",   NULL,                OFFSET(disto_alloc),   AV_OPT_TYPE_INT,   { .i64 = 1           }, 0,         1,           VE                },
>      { "fixed_alloc",   NULL,                OFFSET(fixed_alloc),   AV_OPT_TYPE_INT,   { .i64 = 0           }, 0,         1,           VE                },
>      { "fixed_quality", NULL,                OFFSET(fixed_quality), AV_OPT_TYPE_INT,   { .i64 = 0           }, 0,         1,           VE                },
> +    { "shr",           NULL,                OFFSET(shr),           AV_OPT_TYPE_INT,   { .i64 = 0           }, 0,         7,           VE                },
>      { NULL },
>  };
>
> --
> 1.7.10.2 (Apple Git-33)
>

Patch is silly. It adds option to shift pixel values by some random
unlimited number.
Even feature is far from being useful at all and it is not documented at all.

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.

Shifting each pixel is stupid and slow, if shifting was not required
when storing
<16bps in 16 bps pixel format than I would recommend using 16bit pixel
format and bunch of
other pixel formats would simply not exist but reality is different.