diff mbox series

[FFmpeg-devel] avcodec/jpeglsdec: Don't allocate+free JPEGLSState for every frame

Message ID HE1PR0301MB21548FFB5887918B425CF0908F4E9@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit 9b3c46a081a9f01559082bf7a154fc6be1e06c18
Headers show
Series [FFmpeg-devel] avcodec/jpeglsdec: Don't allocate+free JPEGLSState for every frame
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 14, 2021, 9:21 p.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/jpeglsdec.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Andreas Rheinhardt April 16, 2021, 11:09 a.m. UTC | #1
Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/jpeglsdec.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/jpeglsdec.c b/libavcodec/jpeglsdec.c
> index 69980eaa49..92df81600b 100644
> --- a/libavcodec/jpeglsdec.c
> +++ b/libavcodec/jpeglsdec.c
> @@ -45,6 +45,11 @@
>   */
>  //#define JLS_BROKEN
>  
> +typedef struct JpegLSDecodeContext {
> +    MJpegDecodeContext mjpeg;
> +    JLSState state;
> +} JpegLSDecodeContext;
> +
>  /**
>   * Decode LSE block with initialization parameters
>   */
> @@ -350,7 +355,7 @@ int ff_jpegls_decode_picture(MJpegDecodeContext *s, int near,
>  {
>      int i, t = 0;
>      uint8_t *zero, *last, *cur;
> -    JLSState *state;
> +    JLSState *const state = &((JpegLSDecodeContext*)s)->state;
>      int off = 0, stride = 1, width, shift, ret = 0;
>      int decoded_height = 0;
>  
> @@ -360,12 +365,8 @@ int ff_jpegls_decode_picture(MJpegDecodeContext *s, int near,
>      last = zero;
>      cur  = s->picture_ptr->data[0];
>  
> -    state = av_mallocz(sizeof(JLSState));
> -    if (!state) {
> -        av_free(zero);
> -        return AVERROR(ENOMEM);
> -    }
>      /* initialize JPEG-LS state from JPEG parameters */
> +    memset(state, 0, sizeof(*state));
>      state->near   = near;
>      state->bpp    = (s->bits < 2) ? 2 : s->bits;
>      state->maxval = s->maxval;
> @@ -537,7 +538,6 @@ int ff_jpegls_decode_picture(MJpegDecodeContext *s, int near,
>      }
>  
>  end:
> -    av_free(state);
>      av_free(zero);
>  
>      return ret;
> @@ -548,7 +548,7 @@ AVCodec ff_jpegls_decoder = {
>      .long_name      = NULL_IF_CONFIG_SMALL("JPEG-LS"),
>      .type           = AVMEDIA_TYPE_VIDEO,
>      .id             = AV_CODEC_ID_JPEGLS,
> -    .priv_data_size = sizeof(MJpegDecodeContext),
> +    .priv_data_size = sizeof(JpegLSDecodeContext),
>      .init           = ff_mjpeg_decode_init,
>      .close          = ff_mjpeg_decode_end,
>      .receive_frame  = ff_mjpeg_receive_frame,
> 
Will apply tomorrow unless there are objections.

- Andreas
Michael Niedermayer April 20, 2021, 3:34 p.m. UTC | #2
On Wed, Apr 14, 2021 at 11:21:40PM +0200, Andreas Rheinhardt wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/jpeglsdec.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
[...]
> @@ -548,7 +548,7 @@ AVCodec ff_jpegls_decoder = {
>      .long_name      = NULL_IF_CONFIG_SMALL("JPEG-LS"),
>      .type           = AVMEDIA_TYPE_VIDEO,
>      .id             = AV_CODEC_ID_JPEGLS,
> -    .priv_data_size = sizeof(MJpegDecodeContext),
> +    .priv_data_size = sizeof(JpegLSDecodeContext),
>      .init           = ff_mjpeg_decode_init,
>      .close          = ff_mjpeg_decode_end,

Sorry for not spotting this before it was applied but this feels problematic
a mjpeg could contain jpeg-ls
in fact iam pretty sure you can mix lossless and lossy coding in a single jpeg
image. Not sure if there is something in jpeg-ls that would prevent it from
being part of the mix.
I think its easier/more robust if the same context type would be used

Thanks

[...]
Andreas Rheinhardt April 20, 2021, 3:47 p.m. UTC | #3
Michael Niedermayer:
> On Wed, Apr 14, 2021 at 11:21:40PM +0200, Andreas Rheinhardt wrote:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/jpeglsdec.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
> [...]
>> @@ -548,7 +548,7 @@ AVCodec ff_jpegls_decoder = {
>>      .long_name      = NULL_IF_CONFIG_SMALL("JPEG-LS"),
>>      .type           = AVMEDIA_TYPE_VIDEO,
>>      .id             = AV_CODEC_ID_JPEGLS,
>> -    .priv_data_size = sizeof(MJpegDecodeContext),
>> +    .priv_data_size = sizeof(JpegLSDecodeContext),
>>      .init           = ff_mjpeg_decode_init,
>>      .close          = ff_mjpeg_decode_end,
> 
> Sorry for not spotting this before it was applied but this feels problematic
> a mjpeg could contain jpeg-ls
> in fact iam pretty sure you can mix lossless and lossy coding in a single jpeg
> image. Not sure if there is something in jpeg-ls that would prevent it from
> being part of the mix.
> I think its easier/more robust if the same context type would be used
> 
I am now looking at this again and there is indeed nothing that prevents
other codecs using ff_mjpeg_decode_init() from calling
ff_jpegls_decode_picture(). I don't know why I didn't check this
earlier, sorry for that. And thanks. Will fix it.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/jpeglsdec.c b/libavcodec/jpeglsdec.c
index 69980eaa49..92df81600b 100644
--- a/libavcodec/jpeglsdec.c
+++ b/libavcodec/jpeglsdec.c
@@ -45,6 +45,11 @@ 
  */
 //#define JLS_BROKEN
 
+typedef struct JpegLSDecodeContext {
+    MJpegDecodeContext mjpeg;
+    JLSState state;
+} JpegLSDecodeContext;
+
 /**
  * Decode LSE block with initialization parameters
  */
@@ -350,7 +355,7 @@  int ff_jpegls_decode_picture(MJpegDecodeContext *s, int near,
 {
     int i, t = 0;
     uint8_t *zero, *last, *cur;
-    JLSState *state;
+    JLSState *const state = &((JpegLSDecodeContext*)s)->state;
     int off = 0, stride = 1, width, shift, ret = 0;
     int decoded_height = 0;
 
@@ -360,12 +365,8 @@  int ff_jpegls_decode_picture(MJpegDecodeContext *s, int near,
     last = zero;
     cur  = s->picture_ptr->data[0];
 
-    state = av_mallocz(sizeof(JLSState));
-    if (!state) {
-        av_free(zero);
-        return AVERROR(ENOMEM);
-    }
     /* initialize JPEG-LS state from JPEG parameters */
+    memset(state, 0, sizeof(*state));
     state->near   = near;
     state->bpp    = (s->bits < 2) ? 2 : s->bits;
     state->maxval = s->maxval;
@@ -537,7 +538,6 @@  int ff_jpegls_decode_picture(MJpegDecodeContext *s, int near,
     }
 
 end:
-    av_free(state);
     av_free(zero);
 
     return ret;
@@ -548,7 +548,7 @@  AVCodec ff_jpegls_decoder = {
     .long_name      = NULL_IF_CONFIG_SMALL("JPEG-LS"),
     .type           = AVMEDIA_TYPE_VIDEO,
     .id             = AV_CODEC_ID_JPEGLS,
-    .priv_data_size = sizeof(MJpegDecodeContext),
+    .priv_data_size = sizeof(JpegLSDecodeContext),
     .init           = ff_mjpeg_decode_init,
     .close          = ff_mjpeg_decode_end,
     .receive_frame  = ff_mjpeg_receive_frame,