[FFmpeg-devel] [PATCH] J2K encoder: fix bpp and add support for 9, 10, and 16 bit YUV
Jean First
jeanfirst at gmail.com
Tue Nov 29 09:03:16 CET 2011
On Tue Nov 29 2011 03:06:00 GMT+0100 (CET), Michael Bradshaw wrote:
> On Mon, Nov 28, 2011 at 2:20 PM, Jean First<jeanfirst at gmail.com> wrote:
>
>> On Mon Nov 28 2011 21:33:39 GMT+0100 (CET), Michael Bradshaw wrote:
>>
>>> The other patch is for encoding 9, 10, and 16 bit YUV video. One
>>> thing: right now I'm using PIX_FMT_YUV420P9 and the like, rather than
>>> the specific big endian and little endian versions. If this is a
>>> problem, or if it would be better to support both big and little
>>> endian formats (I don't know what the industry "standard" is), let me
>>> know and I can add support. It's pretty slow at encoding, just so you
>>> know.
>>>
>> From f3b8d60ecbb745fe3d5c614fa3e599**03459a0e85 Mon Sep 17 00:00:00 2001
>>> From: Michael Bradshaw<mbradshaw at sorensonmedia.com>
>>> Date: Mon, 28 Nov 2011 13:19:55 -0700
>>> Subject: [PATCH 103/103] Added support for encoding 9, 10, and 16 bit YUV
>>> J2K
>>> video
>>>
>>>
>>> Signed-off-by: Michael Bradshaw<mbradshaw at sorensonmedia.com>
>>> ---
>>> libavcodec/libopenjpegenc.c | 67 ++++++++++++++++++++++++++++++**
>>> +++++++++++--
>>> 1 files changed, 64 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
>>> index 8403ca1..dffd6bf 100644
>>> --- a/libavcodec/libopenjpegenc.c
>>> +++ b/libavcodec/libopenjpegenc.c
>>> @@ -93,6 +93,27 @@ static opj_image_t *mj2_create_image(**AVCodecContext
>>> *avctx, opj_cparameters_t *p
>>> color_space = CLRSPC_SYCC;
>>> numcomps = 3;
>>> break;
>>> + case PIX_FMT_YUV420P9:
>>> + case PIX_FMT_YUV422P9:
>>> + case PIX_FMT_YUV444P9:
>>> + color_space = CLRSPC_SYCC;
>>> + numcomps = 3;
>>> + bpp = 9;
>>> + break;
>>> + case PIX_FMT_YUV420P10:
>>> + case PIX_FMT_YUV422P10:
>>> + case PIX_FMT_YUV444P10:
>>> + color_space = CLRSPC_SYCC;
>>>
>> indent.
>
> Fixed. Good catch.
Well it was rather obvioius :-).
> + numcomps = 3;
>>> + bpp = 10;
>>> + break;
>>> + case PIX_FMT_YUV420P16:
>>> + case PIX_FMT_YUV422P16:
>>> + case PIX_FMT_YUV444P16:
>>> + color_space = CLRSPC_SYCC;
>>> + numcomps = 3;
>>> + bpp = 16;
>>> + break;
>>> default:
>>> av_log(avctx, AV_LOG_ERROR, "The requested pixel format '%s' is
>>> not supported\n", av_get_pix_fmt_name(avctx->**pix_fmt));
>>> return NULL;
>>> @@ -182,7 +203,7 @@ static int libopenjpeg_copy_rgba(**AVCodecContext
>>> *avctx, AVFrame *frame, opj_imag
>>> return 1;
>>> }
>>>
>>> -static int libopenjpeg_copy_yuv(**AVCodecContext *avctx, AVFrame
>>> *frame, opj_image_t *image)
>>> +static int libopenjpeg_copy_yuv8(**AVCodecContext *avctx, AVFrame
>>> *frame, opj_image_t *image)
>>> {
>>> int compno;
>>> int x;
>>> @@ -210,6 +231,36 @@ static int libopenjpeg_copy_yuv(**AVCodecContext
>>> *avctx, AVFrame *frame, opj_image
>>> return 1;
>>> }
>>>
>>> +static int libopenjpeg_copy_yuv16(**AVCodecContext *avctx, AVFrame
>>> *frame, opj_image_t *image)
>>>
>> libopenjpeg_copy_yuv8 and libopenjpeg_copy_yuv16 look quite similar. maybe
>> it's possible to merge them ?
>>
> They are very similar. I don't know how to merge them any better though.
> yuv8 iterates over a uint8_t* buffer, whereas yuv16 iterates over a
> uint16_t*. I suppose I could merge parts of them, but the core part of the
> functions (the for loops that actually copy the buffer) can't be merged,
> AFAIK. Feedback is appreciated though if you see a good way to do this.
It's not that important - I'd rather not refactor then have some
unreadable spaghetti.
>
>> @@ -300,6 +362,5 @@ AVCodec ff_libopenjpeg_encoder = {
>>> .close = libopenjpeg_encode_close,
>>> .decode = NULL,
>>> .capabilities = 0,
>>> - .pix_fmts = (const enum PixelFormat[]){PIX_FMT_GRAY8,**
>>> PIX_FMT_RGB24,PIX_FMT_RGBA,**PIX_FMT_YUV420P,PIX_FMT_**
>>> YUV422P,PIX_FMT_YUV440P,PIX_**FMT_YUV444P},
>>>
>> why do you remove .pix_fmts ? I'd rather update and reformat it.
>>
> It was just getting long and it meant maintaining the list of supported
> pixel formats in 3 places rather than just two (right now it's just in two
> switches). I'll add it back in though.
>
>
>> .long_name = NULL_IF_CONFIG_SMALL("OpenJPEG based JPEG 2000 encoder"),
>>> } ;
>>> --
>>> 1.7.7
>>>
>>>
>> by the way - there are some quite long lines that could be wraped.
>>
> I'll try and keep them shorter and possibly fix some of the currently
> longer ones in a future patch. Attached is a patch that fixes the two
> issues discussed.
nice.
And while at it will improve the readability alot if you f.ex align the
AVCodec struct at the = signs and align the code at some other places
where the code looks like a block.
[...]
> From 5d018d9e322c30580c929acee54754f39fefc528 Mon Sep 17 00:00:00 2001
> From: Michael Bradshaw <mbradshaw at sorensonmedia.com>
> Date: Mon, 28 Nov 2011 18:51:19 -0700
> Subject: [PATCH 104/104] Fixed a formatting issue on one line and
> added the
> .pix_fmts line back in
>
>
> Signed-off-by: Michael Bradshaw <mbradshaw at sorensonmedia.com>
> ---
> libavcodec/libopenjpegenc.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
> index dffd6bf..23407b2 100644
> --- a/libavcodec/libopenjpegenc.c
> +++ b/libavcodec/libopenjpegenc.c
> @@ -103,7 +103,7 @@ static opj_image_t
> *mj2_create_image(AVCodecContext *avctx, opj_cparameters_t *p
> case PIX_FMT_YUV420P10:
> case PIX_FMT_YUV422P10:
> case PIX_FMT_YUV444P10:
> - color_space = CLRSPC_SYCC;
> + color_space = CLRSPC_SYCC;
> numcomps = 3;
> bpp = 10;
> break;
> @@ -362,5 +362,8 @@ AVCodec ff_libopenjpeg_encoder = {
> .close = libopenjpeg_encode_close,
> .decode = NULL,
> .capabilities = 0,
> + .pix_fmts = (const enum
> PixelFormat[]){PIX_FMT_GRAY8,PIX_FMT_RGB24,PIX_FMT_RGBA,PIX_FMT_YUV420P,
> +
> PIX_FMT_YUV422P,PIX_FMT_YUV440P,PIX_FMT_YUV444P,PIX_FMT_YUV420P9,PIX_FMT_YUV422P9,PIX_FMT_YUV444P9,
> +
> PIX_FMT_YUV420P10,PIX_FMT_YUV422P10,PIX_FMT_YUV444P10,PIX_FMT_YUV420P16,PIX_FMT_YUV422P16,PIX_FMT_YUV444P16},
> .long_name = NULL_IF_CONFIG_SMALL("OpenJPEG based JPEG 2000
> encoder"),
> } ;
> --
> 1.7.7
I think you get a lot of karma if writing it like this (or similar) - if
you put PIX_FMT_RGB24 at the filrst place, it will
be the first choice for AVFilter swscale autoinserting...
.pix_fmts = (const enum PixelFormat[]){ PIX_FMT_RGB24,
PIX_FMT_RGBA,
PIX_FMT_GRAY8,
PIX_FMT_YUV420P,
PIX_FMT_YUV422P,
PIX_FMT_YUV440P,
PIX_FMT_YUV444P,
PIX_FMT_YUV420P9,
PIX_FMT_YUV422P9, PIX_FMT_YUV444P9,
PIX_FMT_YUV420P10,
PIX_FMT_YUV422P10, PIX_FMT_YUV444P10,
PIX_FMT_YUV420P16,
PIX_FMT_YUV422P16, PIX_FMT_YUV444P16 },
More information about the ffmpeg-devel
mailing list