diff mbox

[FFmpeg-devel] libopenjpegenc: recreate image data buffer after encoding frame

Message ID cfb79759-d827-9b73-2dbb-190cebec0d8f@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Oct. 13, 2016, 7:21 p.m. UTC
On 13.10.2016 03:36, Michael Bradshaw wrote:
> Thanks for that (and the link back to the OpenJPEG source). Well dang. I
> think it would be better to change the patch to completely remove the image
> member from LibOpenJPEGContext, and instead just create a local image (and
> destroy it) for every call to libopenjpeg_encode_frame.

OK. Attached patch does that for openjpeg 2.
I didn't change the behavior for openjpeg 1, as reusing the image works there.

Best regards,
Andreas

Comments

Michael Bradshaw Oct. 14, 2016, 4:07 a.m. UTC | #1
On Thu, Oct 13, 2016 at 12:21 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:
>
> OK. Attached patch does that for openjpeg 2.
> I didn't change the behavior for openjpeg 1, as reusing the image works
> there.


Looks good to me. Thanks! Please feel free to apply it.
Andreas Cadhalpun Oct. 14, 2016, 3:01 p.m. UTC | #2
On 14.10.2016 06:07, Michael Bradshaw wrote:
> On Thu, Oct 13, 2016 at 12:21 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
>>
>> OK. Attached patch does that for openjpeg 2.
>> I didn't change the behavior for openjpeg 1, as reusing the image works
>> there.
> 
> 
> Looks good to me. Thanks! Please feel free to apply it.

Pushed.

Best regards,
Andreas
diff mbox

Patch

From 350ceab27db0346d89698070e40b1ae19ff1d559 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Thu, 13 Oct 2016 21:16:35 +0200
Subject: [PATCH] libopenjpegenc: stop reusing image data buffer for openjpeg 2

openjpeg 2 sets the data pointers of the image components to NULL,
causing segfaults if the image is reused.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavcodec/libopenjpegenc.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 5042507..857ee1a 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -52,7 +52,9 @@ 
 
 typedef struct LibOpenJPEGContext {
     AVClass *avclass;
+#if OPENJPEG_MAJOR_VERSION == 1
     opj_image_t *image;
+#endif // OPENJPEG_MAJOR_VERSION == 1
     opj_cparameters_t enc_params;
 #if OPENJPEG_MAJOR_VERSION == 1
     opj_event_mgr_t event_mgr;
@@ -369,18 +371,22 @@  static av_cold int libopenjpeg_encode_init(AVCodecContext *avctx)
         cinema_parameters(&ctx->enc_params);
     }
 
+#if OPENJPEG_MAJOR_VERSION == 1
     ctx->image = mj2_create_image(avctx, &ctx->enc_params);
     if (!ctx->image) {
         av_log(avctx, AV_LOG_ERROR, "Error creating the mj2 image\n");
         err = AVERROR(EINVAL);
         goto fail;
     }
+#endif // OPENJPEG_MAJOR_VERSION == 1
 
     return 0;
 
 fail:
+#if OPENJPEG_MAJOR_VERSION == 1
     opj_image_destroy(ctx->image);
     ctx->image = NULL;
+#endif // OPENJPEG_MAJOR_VERSION == 1
     return err;
 }
 
@@ -591,19 +597,25 @@  static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
                                     const AVFrame *frame, int *got_packet)
 {
     LibOpenJPEGContext *ctx = avctx->priv_data;
-    opj_image_t *image      = ctx->image;
+    int ret;
+    AVFrame *gbrframe;
+    int cpyresult = 0;
 #if OPENJPEG_MAJOR_VERSION == 1
+    opj_image_t *image      = ctx->image;
     opj_cinfo_t *compress   = NULL;
     opj_cio_t *stream       = NULL;
     int len;
 #else // OPENJPEG_MAJOR_VERSION == 2
+    PacketWriter writer     = { 0 };
     opj_codec_t *compress   = NULL;
     opj_stream_t *stream    = NULL;
-    PacketWriter writer     = { 0 };
+    opj_image_t *image      = mj2_create_image(avctx, &ctx->enc_params);
+    if (!image) {
+        av_log(avctx, AV_LOG_ERROR, "Error creating the mj2 image\n");
+        ret = AVERROR(EINVAL);
+        goto done;
+    }
 #endif // OPENJPEG_MAJOR_VERSION == 1
-    int cpyresult = 0;
-    int ret;
-    AVFrame *gbrframe;
 
     switch (avctx->pix_fmt) {
     case AV_PIX_FMT_RGB24:
@@ -626,8 +638,10 @@  static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
     case AV_PIX_FMT_GBRP14:
     case AV_PIX_FMT_GBRP16:
         gbrframe = av_frame_clone(frame);
-        if (!gbrframe)
-            return AVERROR(ENOMEM);
+        if (!gbrframe) {
+            ret = AVERROR(ENOMEM);
+            goto done;
+        }
         gbrframe->data[0] = frame->data[2]; // swap to be rgb
         gbrframe->data[1] = frame->data[0];
         gbrframe->data[2] = frame->data[1];
@@ -684,19 +698,21 @@  static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
         av_log(avctx, AV_LOG_ERROR,
                "The frame's pixel format '%s' is not supported\n",
                av_get_pix_fmt_name(avctx->pix_fmt));
-        return AVERROR(EINVAL);
+        ret = AVERROR(EINVAL);
+        goto done;
         break;
     }
 
     if (!cpyresult) {
         av_log(avctx, AV_LOG_ERROR,
                "Could not copy the frame data to the internal image buffer\n");
-        return -1;
+        ret = -1;
+        goto done;
     }
 
 #if OPENJPEG_MAJOR_VERSION == 2
     if ((ret = ff_alloc_packet2(avctx, pkt, 1024, 0)) < 0) {
-        return ret;
+        goto done;
     }
 #endif // OPENJPEG_MAJOR_VERSION == 2
 
@@ -763,7 +779,7 @@  static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
 #error Missing call to opj_stream_set_user_data
 #endif
 
-    if (!opj_start_compress(compress, ctx->image, stream) ||
+    if (!opj_start_compress(compress, image, stream) ||
         !opj_encode(compress, stream) ||
         !opj_end_compress(compress, stream)) {
         av_log(avctx, AV_LOG_ERROR, "Error during the opj encode\n");
@@ -782,6 +798,7 @@  done:
 #if OPENJPEG_MAJOR_VERSION == 2
     opj_stream_destroy(stream);
     opj_destroy_codec(compress);
+    opj_image_destroy(image);
 #else
     opj_cio_close(stream);
     opj_destroy_compress(compress);
@@ -791,10 +808,12 @@  done:
 
 static av_cold int libopenjpeg_encode_close(AVCodecContext *avctx)
 {
+#if OPENJPEG_MAJOR_VERSION == 1
     LibOpenJPEGContext *ctx = avctx->priv_data;
 
     opj_image_destroy(ctx->image);
     ctx->image = NULL;
+#endif // OPENJPEG_MAJOR_VERSION == 1
     return 0;
 }
 
-- 
2.9.3