| Message ID | 20190607214533.20540-1-ullysses.a.eoff@intel.com |
|---|---|
| State | New |
| Headers | show |
On 07.06.2019, at 23:45, "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote: > The compound literals assigned to "components" > only exist within the scope of the if/else > block (thanks Mark Thompson for the better > explanation). > > Thus, after this if/else block, "components" > ends up pointing to an arbitrary/undefined > array. With some compilers and depending on > optimization settings, these arbitrary values > may end up being the same value (i.e. 0 with > GNU GCC 9.x). Unfortunately, the GNU GCC > compiler, at least, never prints any warnings > about this. > > This patch fixes this issue by assigning the > constant arrays to local variables at function > scope and then pointing "components" 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(-) > > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c > index 4dcdc3d16bb0..bd029cc90315 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_yuv[3] = { 1, 2, 3 }; Not sure what the necessary scope is for these, but maybe safest to go for "static const"?
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Reimar Döffinger > Sent: Friday, June 07, 2019 11:07 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v4] vaapi_encode_mjpeg: fix bad component id bug > > On 07.06.2019, at 23:45, "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote: > > > The compound literals assigned to "components" > > only exist within the scope of the if/else > > block (thanks Mark Thompson for the better > > explanation). > > > > Thus, after this if/else block, "components" > > ends up pointing to an arbitrary/undefined > > array. With some compilers and depending on > > optimization settings, these arbitrary values > > may end up being the same value (i.e. 0 with > > GNU GCC 9.x). Unfortunately, the GNU GCC > > compiler, at least, never prints any warnings > > about this. > > > > This patch fixes this issue by assigning the > > constant arrays to local variables at function > > scope and then pointing "components" 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(-) > > > > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c > > index 4dcdc3d16bb0..bd029cc90315 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_yuv[3] = { 1, 2, 3 }; > > Not sure what the necessary scope is for these, but maybe safest to go for "static const"? I don't think these need scope beyond this function since the values are just "copied" into the frame header here. So static would just be an [barely detectable] optimization. Unless I am missing something? > _______________________________________________ > 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".
On 07/06/2019 22:45, U. Artie Eoff wrote: > The compound literals assigned to "components" > only exist within the scope of the if/else > block (thanks Mark Thompson for the better > explanation). > > Thus, after this if/else block, "components" > ends up pointing to an arbitrary/undefined > array. With some compilers and depending on > optimization settings, these arbitrary values > may end up being the same value (i.e. 0 with > GNU GCC 9.x). Unfortunately, the GNU GCC > compiler, at least, never prints any warnings > about this. > > This patch fixes this issue by assigning the > constant arrays to local variables at function > scope and then pointing "components" 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(-) > > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c > index 4dcdc3d16bb0..bd029cc90315 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_yuv[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_yuv; > > // Frame header. > > LGTM; applied. Thanks! - Mark
On Friday, 07 June 2019 at 23:45, U. Artie Eoff wrote: > The compound literals assigned to "components" > only exist within the scope of the if/else > block (thanks Mark Thompson for the better > explanation). > > Thus, after this if/else block, "components" > ends up pointing to an arbitrary/undefined > array. With some compilers and depending on > optimization settings, these arbitrary values > may end up being the same value (i.e. 0 with > GNU GCC 9.x). Unfortunately, the GNU GCC > compiler, at least, never prints any warnings > about this. > > This patch fixes this issue by assigning the > constant arrays to local variables at function > scope and then pointing "components" to those > as necessary. > > Fixes #7915 Brilliant detective work, Artie. Could you open a bug report with gcc upstream? A case like this should get an explicit warning from gcc like you pointed out. Regards, Dominik
Dominik 'Rathann' Mierzejewski: > On Friday, 07 June 2019 at 23:45, U. Artie Eoff wrote: >> The compound literals assigned to "components" >> only exist within the scope of the if/else >> block (thanks Mark Thompson for the better >> explanation). >> >> Thus, after this if/else block, "components" >> ends up pointing to an arbitrary/undefined >> array. With some compilers and depending on >> optimization settings, these arbitrary values >> may end up being the same value (i.e. 0 with >> GNU GCC 9.x). Unfortunately, the GNU GCC >> compiler, at least, never prints any warnings >> about this. >> >> This patch fixes this issue by assigning the >> constant arrays to local variables at function >> scope and then pointing "components" to those >> as necessary. >> >> Fixes #7915 > > Brilliant detective work, Artie. Could you open a bug report with gcc > upstream? A case like this should get an explicit warning from gcc like > you pointed out. > > Regards, > Dominik > A request for this has already been opened [1]. It seems that this is also responsible for filesystem corruption on linux when using a SSD cache + HDD combination (see [2]). - Andreas [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89990 [2]: https://bugzilla.kernel.org/show_bug.cgi?id=203573
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Andreas Rheinhardt > Sent: Thursday, June 13, 2019 2:11 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v4] vaapi_encode_mjpeg: fix bad component id bug > > Dominik 'Rathann' Mierzejewski: > > On Friday, 07 June 2019 at 23:45, U. Artie Eoff wrote: > >> The compound literals assigned to "components" > >> only exist within the scope of the if/else > >> block (thanks Mark Thompson for the better > >> explanation). > >> > >> Thus, after this if/else block, "components" > >> ends up pointing to an arbitrary/undefined > >> array. With some compilers and depending on > >> optimization settings, these arbitrary values > >> may end up being the same value (i.e. 0 with > >> GNU GCC 9.x). Unfortunately, the GNU GCC > >> compiler, at least, never prints any warnings > >> about this. > >> > >> This patch fixes this issue by assigning the > >> constant arrays to local variables at function > >> scope and then pointing "components" to those > >> as necessary. > >> > >> Fixes #7915 > > > > Brilliant detective work, Artie. Could you open a bug report with gcc > > upstream? A case like this should get an explicit warning from gcc like > > you pointed out. > > > > Regards, > > Dominik > > > A request for this has already been opened [1]. > It seems that this is also responsible for filesystem corruption on > linux when using a SSD cache + HDD combination (see [2]). > According to comments in the gcc request, it looks like we could use " -fsanitize=address". Anyone care to test it out to see if it will work? > - Andreas > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89990 > [2]: https://bugzilla.kernel.org/show_bug.cgi?id=203573 > _______________________________________________ > 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".
On Thu, Jun 13, 2019 at 15:41:55 +0000, Eoff, Ullysses A wrote: > According to comments in the gcc request, it looks like we > could use " -fsanitize=address". Anyone care to test it > out to see if it will work? The AddressSanatizer does not make this a warning at compile time (or perhaps it may, additionally), but it instruments the binary with code for analyzing and debugging address accesses, similar to valgrind: https://github.com/google/sanitizers/wiki/AddressSanitizer So this is not a permanent solution, but might be something for fate. Moritz
diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c index 4dcdc3d16bb0..bd029cc90315 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_yuv[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_yuv; // Frame header.
The compound literals assigned to "components" only exist within the scope of the if/else block (thanks Mark Thompson for the better explanation). Thus, after this if/else block, "components" ends up pointing to an arbitrary/undefined array. With some compilers and depending on optimization settings, these arbitrary values may end up being the same value (i.e. 0 with GNU GCC 9.x). Unfortunately, the GNU GCC compiler, at least, never prints any warnings about this. This patch fixes this issue by assigning the constant arrays to local variables at function scope and then pointing "components" 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(-)