[FFmpeg-devel] [PATCH] libopenjpeg J2K encoder

[Carl Eugen Hoyos]

Michael Bradshaw <mbradshaw <at> sorensonmedia.com> writes:

> Hi, attached is a patch for J2K encoding with libopenjpeg.  Here are a
> couple of particular features you might want to be aware of:
> Supported pixel formats: Gray, RGB24, RGBA, YUV420P, YUV422P, YUV440P, and
> YUV444P

Did you try to support GBR24P as input format?
If I understand your code correctly, it should be significantly easier to
support than RGB*. You find a sample in ticket #297.

> (though decoding YUV444P is tricky since it looks like RGB24)

Are you describing a bug in our decoder, or a limitation of libopenjpeg (or of
the whole standard)?

[...]

> libavcodec/libopenjpeg.c    |  226 ---------------------------------
> libavcodec/libopenjpegdec.c |  226 +++++++++++++++++++++++++++++++++

Is this necessary?
I am not against it, but we currently have tiff.c and tiffenc.c and so far
nobody claimed it is too difficult to find the correct file;-)
In any case, I believe this should be a separate patch (to make this one smaller
and easier to review).

[...]

> +++ b/libavcodec/libopenjpegenc.c
> @@ -0,0 +1,293 @@
> +/*
> + * JPEG 2000 decoding support via OpenJPEG
> + * Copyright (c) 2009 Jaikrishnan Menon <realityman at gmx.net>

This looks wrong.

[...]

> +        av_log(avctx, AV_LOG_ERROR, "Error creating the copressor\n");

typo

[...]

> +        av_free(ctx->compress);
> +        ctx->compress = NULL;

The advantage of av_freep is that it makes the second line unnecessary.
(Many more below.)

[...]

> +    switch (avctx->pix_fmt) {
> +        case PIX_FMT_GRAY8:
> +            result = libopenjpeg_copy_rgba(avctx, frame, image, 1);
> +            break;
>
> +        case PIX_FMT_YUV420P:
> +        case PIX_FMT_YUV422P:
> +        case PIX_FMT_YUV440P:
> +        case PIX_FMT_YUV444P:
> +            result = libopenjpeg_copy_yuv(avctx, frame, image);

Sorry if I misunderstand the code, but I don't think the frames should be copied
for above cases. Or do I miss something?

> +        case PIX_FMT_RGB24:
> +            result = libopenjpeg_copy_rgba(avctx, frame, image, 3);
> +            break;
> +        case PIX_FMT_RGBA:
> +            result = libopenjpeg_copy_rgba(avctx, frame, image, 4);
> +            break;

And this may be simpler with GBR24P, but this is just an idea and not very
important.

Some return -1 can be changed into "return AVERROR(ENOMEM);" and "return
AVERROR(EINVAL);" respectively.

Thank you for your contribution, Carl Eugen