[FFmpeg-devel] vaapi_encode_mjpeg: WA: fix bad component id bug

Submitted by U. Artie Eoff on June 7, 2019, 8:28 p.m.

Details

Message ID 20190607202818.25099-1-ullysses.a.eoff@intel.com
State New
Headers show

Commit Message

U. Artie Eoff June 7, 2019, 8:28 p.m.
When compile time optimizations are enabled and
compiling with GNU GCC 9.x, the pointer assignment
to the inline brace-enclosed list initialized
array does not work and "component" ends up pointing
to an empty array.  This is probably a bug in GCC
9.x.

This patch works around this issue by assigning
the constant arrays to local variables and then
pointing "component" to those as necessary.

Fixes #7915

Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
---
 libavcodec/vaapi_encode_mjpeg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Reimar Döffinger June 7, 2019, 8:41 p.m.
On 07.06.2019, at 22:28, "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:

> When compile time optimizations are enabled and
> compiling with GNU GCC 9.x, the pointer assignment
> to the inline brace-enclosed list initialized
> array does not work and "component" ends up pointing
> to an empty array.  This is probably a bug in GCC
> 9.x.

This to me sounds like a serious enough bug that I'm tempted to suggest blacklisting gcc 9 instead...
However the original code doesn't seem really proper to me.
Have you tried replacing (uint8_t[3]) by (const uint8_t[3])?
Mark Thompson June 7, 2019, 8:47 p.m.
On 07/06/2019 21:28, U. Artie Eoff wrote:
> When compile time optimizations are enabled and
> compiling with GNU GCC 9.x, the pointer assignment
> to the inline brace-enclosed list initialized
> array does not work and "component" ends up pointing
> to an empty array.  This is probably a bug in GCC
> 9.x.
> 
> This patch works around this issue by assigning
> the constant arrays to local variables and then
> pointing "component" to those as necessary.
> 
> Fixes #7915
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> ---
>  libavcodec/vaapi_encode_mjpeg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Huh.  No, the compiler is right - the compound literals only exist in the tiny scope of the branches of the if.  I'm somewhat disappointed it isn't warning you about those variables having worked out enough to eliminate them, though - it clearly knows that it's going to generate something undefined.

Good catch, in any case!

> diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c
> index 4dcdc3d16bb0..3e8cbb7c9bf9 100644
> --- a/libavcodec/vaapi_encode_mjpeg.c
> +++ b/libavcodec/vaapi_encode_mjpeg.c
> @@ -227,6 +227,8 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
>      JPEGRawScanHeader                 *sh = &priv->scan.header;
>      VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;
>      const AVPixFmtDescriptor *desc;
> +    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };
> +    const uint8_t components_123[3] = {  1,   2,   3  };

It would probably make more sense to call this components_yuv?

>      const uint8_t *components;
>      int t, i, quant_scale, len;
>  
> @@ -235,9 +237,9 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
>      desc = av_pix_fmt_desc_get(priv->common.input_frames->sw_format);
>      av_assert0(desc);
>      if (desc->flags & AV_PIX_FMT_FLAG_RGB)
> -        components = (uint8_t[3]) { 'R', 'G', 'B' };
> +        components = components_rgb;
>      else
> -        components = (uint8_t[3]) {  1,   2,   3  };
> +        components = components_123;
>  
>      // Frame header.
>  
> 

Thanks,

- Mark
U. Artie Eoff June 7, 2019, 8:52 p.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Reimar Döffinger

> Sent: Friday, June 07, 2019 1:41 PM

> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH] vaapi_encode_mjpeg: WA: fix bad component id bug

> 

> On 07.06.2019, at 22:28, "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:

> 

> > When compile time optimizations are enabled and

> > compiling with GNU GCC 9.x, the pointer assignment

> > to the inline brace-enclosed list initialized

> > array does not work and "component" ends up pointing

> > to an empty array.  This is probably a bug in GCC

> > 9.x.

> 

> This to me sounds like a serious enough bug that I'm tempted to suggest blacklisting gcc 9 instead...

> However the original code doesn't seem really proper to me.

> Have you tried replacing (uint8_t[3]) by (const uint8_t[3])?


Yes I tried that but it didn't fix the issue.

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

> 

> To unsubscribe, visit link above, or email

> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
U. Artie Eoff June 7, 2019, 8:54 p.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Mark Thompson

> Sent: Friday, June 07, 2019 1:47 PM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH] vaapi_encode_mjpeg: WA: fix bad component id bug

> 

> On 07/06/2019 21:28, U. Artie Eoff wrote:

> > When compile time optimizations are enabled and

> > compiling with GNU GCC 9.x, the pointer assignment

> > to the inline brace-enclosed list initialized

> > array does not work and "component" ends up pointing

> > to an empty array.  This is probably a bug in GCC

> > 9.x.

> >

> > This patch works around this issue by assigning

> > the constant arrays to local variables and then

> > pointing "component" to those as necessary.

> >

> > Fixes #7915

> >

> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>

> > ---

> >  libavcodec/vaapi_encode_mjpeg.c | 6 ++++--

> >  1 file changed, 4 insertions(+), 2 deletions(-)

> 

> Huh.  No, the compiler is right - the compound literals only exist in the tiny scope of the branches of the if.  I'm somewhat

> disappointed it isn't warning you about those variables having worked out enough to eliminate them, though - it clearly knows that it's

> going to generate something undefined.

> 


Ah yes, that explanation makes sense.

> Good catch, in any case!

> 

> > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c

> > index 4dcdc3d16bb0..3e8cbb7c9bf9 100644

> > --- a/libavcodec/vaapi_encode_mjpeg.c

> > +++ b/libavcodec/vaapi_encode_mjpeg.c

> > @@ -227,6 +227,8 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,

> >      JPEGRawScanHeader                 *sh = &priv->scan.header;

> >      VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;

> >      const AVPixFmtDescriptor *desc;

> > +    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };

> > +    const uint8_t components_123[3] = {  1,   2,   3  };

> 

> It would probably make more sense to call this components_yuv?

> 


Sure.  V2 coming up.

> >      const uint8_t *components;

> >      int t, i, quant_scale, len;

> >

> > @@ -235,9 +237,9 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,

> >      desc = av_pix_fmt_desc_get(priv->common.input_frames->sw_format);

> >      av_assert0(desc);

> >      if (desc->flags & AV_PIX_FMT_FLAG_RGB)

> > -        components = (uint8_t[3]) { 'R', 'G', 'B' };

> > +        components = components_rgb;

> >      else

> > -        components = (uint8_t[3]) {  1,   2,   3  };

> > +        components = components_123;

> >

> >      // Frame header.

> >

> >

> 

> Thanks,

> 

> - Mark

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

> 

> To unsubscribe, visit link above, or email

> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c
index 4dcdc3d16bb0..3e8cbb7c9bf9 100644
--- a/libavcodec/vaapi_encode_mjpeg.c
+++ b/libavcodec/vaapi_encode_mjpeg.c
@@ -227,6 +227,8 @@  static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
     JPEGRawScanHeader                 *sh = &priv->scan.header;
     VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;
     const AVPixFmtDescriptor *desc;
+    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };
+    const uint8_t components_123[3] = {  1,   2,   3  };
     const uint8_t *components;
     int t, i, quant_scale, len;
 
@@ -235,9 +237,9 @@  static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
     desc = av_pix_fmt_desc_get(priv->common.input_frames->sw_format);
     av_assert0(desc);
     if (desc->flags & AV_PIX_FMT_FLAG_RGB)
-        components = (uint8_t[3]) { 'R', 'G', 'B' };
+        components = components_rgb;
     else
-        components = (uint8_t[3]) {  1,   2,   3  };
+        components = components_123;
 
     // Frame header.