[FFmpeg-devel] [PATCH] libopenjpeg J2K encoder

[Michael Bradshaw]

On Sat, Nov 19, 2011 at 1:00 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Fri, Nov 18, 2011 at 02:42:16PM -0700, Michael Bradshaw wrote:
>> 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.
>
> thanks
> should i merge it already or do you want to do more work on it ?

You can go ahead and merge it now.  I've been playing around testing
with some optimizations (trying to increase cache hits and stuff), but
so far I haven't got anything new worth merging, other than the
patches you've supplied.

I'm going to focus on adding 9, 10, and maybe 16 bit YUV support now
though.  I will let you know when I have these ready.  Hopefully this
week.

>> 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?
>
> We have encoders that use multithreading on slice basis. And we have
> encoders that support multithreading entirely within some seperate
> lib.
> So if you want to implement frame based multithreading you would have
> to add some support for this first to libavcodec or do it entirely in
> the libopenjpeg wraper

Hmm... I'll have to look into it.  Thanks for the info.

Michael