| Message ID | MN2PR13MB27527EFF56729E986F2D2DACBA8F0@MN2PR13MB2752.namprd13.prod.outlook.com |
|---|---|
| State | Withdrawn, archived |
| Headers | show |
On 9/16/2019 10:05 PM, Soft Works wrote: > An int32 option cannot have a maximum of UINT32_MAX AV_OPT_TYPE_INT options are int64_t. In this case however the storage type for trans_color in GifState is int. Reading the code i see it's intended to be uint32_t, so i think the correct fix is changing its storage type, and not limiting its allowed range. Same with stored_bg_color. > > Before this patch > -trans_color <int> .D.V..... color value (ARGB) that is used instead of transparent color (from 0 to UINT32_MAX) > > Afterwards: > -trans_color <int> .D.V..... color value (ARGB) that is used instead of transparent color (from 0 to INT_MAX) > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > libavcodec/gifdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c > index 1906a4c738..4c5246c3d4 100644 > --- a/libavcodec/gifdec.c > +++ b/libavcodec/gifdec.c > @@ -546,7 +546,7 @@ static av_cold int gif_decode_close(AVCodecContext *avctx) > static const AVOption options[] = { > { "trans_color", "color value (ARGB) that is used instead of transparent color", > offsetof(GifState, trans_color), AV_OPT_TYPE_INT, > - {.i64 = GIF_TRANSPARENT_COLOR}, 0, 0xffffffff, > + {.i64 = GIF_TRANSPARENT_COLOR}, 0, INT_MAX, > AV_OPT_FLAG_DECODING_PARAM|AV_OPT_FLAG_VIDEO_PARAM }, > { NULL }, > }; >
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Tuesday, September 17, 2019 3:11 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] Fix gif decoder max option > > On 9/16/2019 10:05 PM, Soft Works wrote: > > An int32 option cannot have a maximum of UINT32_MAX > > AV_OPT_TYPE_INT options are int64_t. In this case however the storage > type for trans_color in GifState is int. > > Reading the code i see it's intended to be uint32_t, so i think the correct fix is > changing its storage type, and not limiting its allowed range. Same with > stored_bg_color. Hi James, Thanks for looking into this. The purpose of this option is to indicate a "replacement color" for transparent pixels. Such a replacement color itself can never have an alpha component. I was unsure how to indicate this. Maybe, by setting the maximum to 0x00FFFFFF ? softworkz
On 9/16/2019 10:24 PM, Soft Works wrote: >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> James Almer >> Sent: Tuesday, September 17, 2019 3:11 AM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH] Fix gif decoder max option >> >> On 9/16/2019 10:05 PM, Soft Works wrote: >>> An int32 option cannot have a maximum of UINT32_MAX >> >> AV_OPT_TYPE_INT options are int64_t. In this case however the storage >> type for trans_color in GifState is int. >> >> Reading the code i see it's intended to be uint32_t, so i think the correct fix is >> changing its storage type, and not limiting its allowed range. Same with >> stored_bg_color. > > Hi James, > > Thanks for looking into this. > > The purpose of this option is to indicate a "replacement color" for transparent pixels. > Such a replacement color itself can never have an alpha component. > > I was unsure how to indicate this. Maybe, by setting the maximum to 0x00FFFFFF ? In that case set the max to GIF_TRANSPARENT_COLOR (default value), which is 0x00FFFFFF. Storage type as int in the struct is still wrong IMO, but i guess it's harmless. I'll let others chime in either way. > > softworkz > _______________________________________________ > 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". >
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Tuesday, September 17, 2019 3:29 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] Fix gif decoder max option > > On 9/16/2019 10:24 PM, Soft Works wrote: > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >> James Almer > >> Sent: Tuesday, September 17, 2019 3:11 AM > >> To: ffmpeg-devel@ffmpeg.org > >> Subject: Re: [FFmpeg-devel] [PATCH] Fix gif decoder max option > >> > >> On 9/16/2019 10:05 PM, Soft Works wrote: > >>> An int32 option cannot have a maximum of UINT32_MAX > >> > >> AV_OPT_TYPE_INT options are int64_t. In this case however the storage > >> type for trans_color in GifState is int. > >> > >> Reading the code i see it's intended to be uint32_t, so i think the > >> correct fix is changing its storage type, and not limiting its > >> allowed range. Same with stored_bg_color. > > > > Hi James, > > > > Thanks for looking into this. > > > > The purpose of this option is to indicate a "replacement color" for > transparent pixels. > > Such a replacement color itself can never have an alpha component. > > > > I was unsure how to indicate this. Maybe, by setting the maximum to > 0x00FFFFFF ? > > In that case set the max to GIF_TRANSPARENT_COLOR (default value), which > is 0x00FFFFFF. > > Storage type as int in the struct is still wrong IMO, but i guess it's harmless. I'll > let others chime in either way. Thanks James, I agree with all your comments. But to be honest, I didn't intend to get into gif decoder development. I had just seen this: $ ffmpeg -h decoder=gif Decoder gif [GIF (Graphics Interchange Format)]: General capabilities: dr1 Threading capabilities: none gif decoder AVOptions: -trans_color <int> .D.V..... color value (ARGB) that is used instead of transparent color (from 0 to UINT32_MAX) (default 1.67772e+07) Two things are incorrect here: The max and the default. My patches are turning this into: -trans_color <int> .D.V..... color value (ARGB) that is used instead of transparent color (from 0 to INT_MAX) (default 16777215) It might not be perfect, but it's much better than before. Changing the storage type would need code changes to the gif decoder and require testing. At this time, I'm afraid, I'm not able to get into this. Of course, I'll do requested detail adjustments to my patches, though. :-) Best regards, softworkz
diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c index 1906a4c738..4c5246c3d4 100644 --- a/libavcodec/gifdec.c +++ b/libavcodec/gifdec.c @@ -546,7 +546,7 @@ static av_cold int gif_decode_close(AVCodecContext *avctx) static const AVOption options[] = { { "trans_color", "color value (ARGB) that is used instead of transparent color", offsetof(GifState, trans_color), AV_OPT_TYPE_INT, - {.i64 = GIF_TRANSPARENT_COLOR}, 0, 0xffffffff, + {.i64 = GIF_TRANSPARENT_COLOR}, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM|AV_OPT_FLAG_VIDEO_PARAM }, { NULL }, };
An int32 option cannot have a maximum of UINT32_MAX Before this patch -trans_color <int> .D.V..... color value (ARGB) that is used instead of transparent color (from 0 to UINT32_MAX) Afterwards: -trans_color <int> .D.V..... color value (ARGB) that is used instead of transparent color (from 0 to INT_MAX) Signed-off-by: softworkz <softworkz@hotmail.com> --- libavcodec/gifdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)