[FFmpeg-devel] [PATCH] libopenjpeg J2K encoder

[Michael Bradshaw]

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

I didn't try to support GBR24P.  Maybe I don't understand FFmpeg's
encoding process, but if the user requests a particular output pixel
format, shouldn't the AVFrame passed in data to the encode function
already be reformatted with the requested pixel format?  (i.e. I'm
wondering how to distinguish between the input and output pixel
formats, as right now I never see the input pixel format).  Those are
the supported output pixel formats.  GBR24P couldn't be supported as
an output, as libopenjpeg expects either gray, RGB, or YUV data.
Maybe I've misunderstood though.

> > (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)?

I'm not sure, actually.  If, when decoding a picture, libopenjpeg
properly sets the color_space variable in the opj_image struct, then
it should be an easy fix in the decoder.  If libopenjpeg doesn't set
that, it's either a problem on their side or a problem with the
standard that prevents them from determining this.  I haven't looked
into libopenjpeg's decoding process enough to tell, but from what I've
seen FFmpeg's decoder has to guess at the pixel format.

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

I just thought about it this morning, and it's not necessary.  I just
saw other files followed that convention and thought I'd jump on the
band wagon.  Looks like that part's already been applied though.

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

Fixed.  I copied and pasted the decoder to use as a reference while
writing the encoder.

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

Fixed.

> > +        av_free(ctx->compress);
> > +        ctx->compress = NULL;
>
> The advantage of av_freep is that it makes the second line unnecessary.
> (Many more below.)

Fixed (in all occurrences).

> > +    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?

For a second, I thought so, but the data in libopenjpeg is stored as
an array of ints, whereas it's an array of unsigned chars in FFmpeg,
so copying them is necessary.

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

See above about GBR24P

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

I think I fixed them.  Make sure I didn't miss any or incorrectly use any.

> Thank you for your contribution, Carl Eugen

Thank you!



@Michael Niedrmayer:
Do you have a particular sample file, and could you tell me the output
size and pixel format?  What are the commands you're passing?  I've
done a little debugging and it looks like I'm not handling the frame's
data properly.  When I tried to encode a video with output size of
300x800, the frame passed to the encode_frame function, I found the
frame had a linesize[0] == 912, not the expected 900 (this was for
RGB24, I haven't looked at the other formats).  Does FFmpeg pad the
data?

>also please remove trailing whitespace

Fixed, I hope.  Thought I got it all the first time but I hadn't.

>the encoder needs to be added in configure so it depends on libopenjpeg and should be aded to doc/general.texi

Fixed (double check the doc/general.texi though).

>You also probably want to add yourself to the copyright at the files top.

Fixed.

>And if you want to maintain this code in the future in ffmpeg then please add yourself to MAINTAINERs too

Done.

Attached is the revised patch.

Thanks,

Michael Bradshaw
Sorenson Media
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-support-for-J2K-encoding-with-libopenjpeg.patch
Type: application/octet-stream
Size: 14196 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111117/cf04d7f3/attachment.obj>