[FFmpeg-devel] [PATCH] libopenjpeg J2K encoder

[Michael Bradshaw]

On Thu, Nov 17, 2011 at 6:11 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Nov 17, 2011 at 05:00:52PM -0700, Michael Bradshaw wrote:
>> On Thu, Nov 17, 2011 at 4:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Thu, Nov 17, 2011 at 11:56:29PM +0100, Michael Niedermayer wrote:
>> >> On Thu, Nov 17, 2011 at 11:42:26PM +0100, Michael Niedermayer wrote:
>> >> > Hi
>> >> >
>> >> > On Thu, Nov 17, 2011 at 03:18:34PM -0700, Michael Bradshaw wrote:
>> >> > > > Michael Bradshaw <mbradshaw <at> sorensonmedia.com> writes:
>> >> [...]
>> >> > > Attached is the revised patch.
>> >> >
>> >> > Ill look at it in a moment, thanks
>> >>
>> >> same gdb crash and under valgrind i get:
>> >>
>> >> Conditional jump or move depends on uninitialised value(s)
>> >>    at 0x6A01E69: opj_event_msg (in /usr/lib/libopenjpeg-2.1.3.0.so)
>> >>    by 0x6A05F7D: j2k_encode (in /usr/lib/libopenjpeg-2.1.3.0.so)
>> >>    by 0x786496: libopenjpeg_encode_frame (libopenjpegenc.c:256)
>> >>    by 0x8A40AE: avcodec_encode_video (utils.c:747)
>> >>    by 0x43F855: output_packet (ffmpeg.c:1324)
>> >>    by 0x443C7E: transcode (ffmpeg.c:2710)
>> >>    by 0x447A55: main (ffmpeg.c:4758)
>> >>
>> >> (i get more stuff from valgrind but the others look unrelated to me)
>> >>
>> >> but otherwise i get a good looking video from valgrinded ffmpeg_g
>> >
>> > following fixes the crash for me:
>> > @@ -34,6 +34,7 @@ typedef struct {
>> >     opj_image_t *image;
>> >     opj_cparameters_t enc_params;
>> >     opj_cinfo_t *compress;
>> > +    opj_event_mgr_t event_mgr;
>> >  } LibOpenJPEGContext;
>> >
>> >  static opj_image_t *mj2_create_image(AVCodecContext *avctx, opj_cparameters_t *parameters)
>> > @@ -160,6 +161,8 @@ static av_cold int libopenjpeg_encode_init(AVCodecContext *avctx)
>> >         return AVERROR(EINVAL);
>> >     }
>> >
>> > +    opj_set_event_mgr(ctx->compress, &ctx->event_mgr, 0);
>> > +
>> >     return 0;
>> >  }
>>
>> You beat me to it.  Thanks for the quick fix.  I've incorporated it
>> (as well as created two functions for the event manager to get
>> feedback from libopenjpeg).  I've also changed the way the copy
>> functions iterate over the buffers so that if linesize != image width
>> the data will still get properly copied.  My 300x800 video test was
>> getting mangled.
>>
>> I've also incremented LIBAVCODEC_VERSION_MINOR, added a line to the
>> Changelog, and found that hiding white space.  See attached for the
>> full patch.
>>
>> About the pixel format layout for identifying YCbCr vs RGB, I think
>> that's something may be useful to those patching the decoder (see
>> http://ffmpeg.org/pipermail/ffmpeg-devel/2011-November/116649.html).
>>
>> Let me know if more changes are needed.  Thanks for all your help in
>> this review process as well!
>
> patch applied, thanks!
>
> To maintain the code, best is to have your own public git clone of
> ffmpeg (for example on github). Where you can freely work and
> integrate patches from others that you reviewed, ...,
> And once you are happy with whats in your repo you tell me to merge it
> and ill merge it then into main ffmpeg.
>
> Ill post a patch or 2 in a moment that you can review/apply if you like

Great, thanks for the support!  I'll review those patches now.  I've
set up a public git clone of FFmpeg (here:
https://github.com/mjbshaw/FFmpeg-OpenJPEG-J2K-Encoder) for
development of this encoder.


@Nicolas George:
FFmpeg's native J2K encoder had fallen into disrepair when I decided
to start this one and support for it had been removed.  It's good to
see it's back.  One of the pros of the OpenJPEG encoder is that it
supports more pixel formats.  One downside is that it's slower (I'll
work on trying to optimize it and multithread it though).

To those familiar with multithreading encoders in FFmpeg, any tips on
what flags or functions I need to define?  Do I just define
CODEC_CAP_FRAME_THREADS and implement init_thread_copy and
update_thread_context functions?

Thanks,

Michael