[FFmpeg-devel] [PATCH] libopenjpeg J2K encoder

[Michael Niedermayer]

On Mon, Nov 21, 2011 at 02:33:45PM -0700, Michael Bradshaw wrote:
> 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.

ok, merged, thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111121/9a7f852f/attachment.asc>