[OpenJDK 2D-Dev] <AWT Dev> RFR: 8081315: 8077982 giflib upgrade breaks system giflib builds with earlier versions

[Alexander Zvegintsev]

Sure, looks good to me.

Thanks,

Alexander.

On 06/05/2015 06:34 PM, Andrew Hughes wrote:
> ----- Original Message -----
>> Hi Andrew,
>>
>> We have removed a workaround[0] for interlaced images support, there is
>> no need in it after 5.0.0 [1]:
>>> DGifSlurp() and EGifSpew() now read and write interlaced images properly.
>> I didn't test it, but it seems to me that interlaced images will look
>> bad after building against GIFLIB < 5.
>> So I suggest to revert SplashDecodeGif() to its state before GIFLIB
>> upgrade and make "interlaced if" conditional[3]:
>>
>> diff -r 58535e641739
>> src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c
>> --- a/src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c
>> +++ b/src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c
>> @@ -213,16 +213,14 @@ SplashDecodeGif(Splash * splash, GifFile
>>                byte_t *pSrc = image->RasterBits;
>>                ImageFormat srcFormat;
>>                ImageRect srcRect, dstRect;
>> -            int pass, npass;
>> +            int pass = 4, npass = 5;
>>
>> +#if GIFLIB_MAJOR < 5
>>                if (desc->Interlace) {
>>                    pass = 0;
>>                    npass = 4;
>>                }
>> -            else {
>> -                pass = 4;
>> -                npass = 5;
>> -            }
>> +#endif
>>
>>                srcFormat.colorMap = colorMapBuf;
>>                srcFormat.depthBytes = 1;
>>
>> And again, I haven't tested it against GIFLIB 4.*.
>>
>> [0]
>> http://cr.openjdk.java.net/~azvegint/jdk/9/8077982/00/src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c.sdiff.html
>> [1] http://sourceforge.net/p/giflib/code/ci/master/tree/NEWS
>> [3] http://cr.openjdk.java.net/~azvegint/jdk/9/8081315/03/
>>
>> Thanks,
>>
>> Alexander.
>>
> Thanks for the heads up on this. It's not very obvious from the code.
>
> I went back through the original 8077982 changes to that file and
> produced the resulting webrev:
>
> http://cr.openjdk.java.net/~andrew/8081315/webrev.03/
>
> Comparing with the original (giflib < 5) splashscreen_gif.c now gives:
>
> @@ -213,16 +213,16 @@
>               byte_t *pSrc = image->RasterBits;
>               ImageFormat srcFormat;
>               ImageRect srcRect, dstRect;
> -            int pass, npass;
> +            int pass = 4, npass = 5;
>   
> +#if GIFLIB_MAJOR < 5
> +	    /* Interlaced gif support is broken in giflib < 5
> +	       so we need to work around this */
>               if (desc->Interlace) {
>                   pass = 0;
>                   npass = 4;
>               }
> -            else {
> -                pass = 4;
> -                npass = 5;
> -            }
> +#endif
>   
>               srcFormat.colorMap = colorMapBuf;
>               srcFormat.depthBytes = 1;
>
> much as you have above.
>
> Ok to push this version?
>
> Thanks,