diff mbox

[FFmpeg-devel] Fix gif decoder max option

Message ID MN2PR13MB27527EFF56729E986F2D2DACBA8F0@MN2PR13MB2752.namprd13.prod.outlook.com
State New
Headers show

Commit Message

Soft Works Sept. 17, 2019, 1:05 a.m. UTC
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(-)

Comments

James Almer Sept. 17, 2019, 1:10 a.m. UTC | #1
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 },
>  };
>
Soft Works Sept. 17, 2019, 1:24 a.m. UTC | #2
> 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
James Almer Sept. 17, 2019, 1:29 a.m. UTC | #3
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".
>
Soft Works Sept. 17, 2019, 2:01 a.m. UTC | #4
> -----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 mbox

Patch

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 },
 };