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

[Alexander Zvegintsev]

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.

On 06/03/2015 03:34 AM, Andrew Hughes wrote:
> ----- Original Message -----
>> I think you meant the subject to be "RFR: 8081315 ..."
>>
> Ah yes, I moved the 8077982 in front of the colon when it's actually part
> of the bug description i.e. RFR: 8081315: 8077982 giflib upgrade :(
>
>> if (DGifCloseFile(gif, NULL) == GIF_ERROR)
>>             return 0;
>>
>> I suppose we probably ought to be checking the (new) error returns
>> although I am not sure what impact a failure on this particular
>> call would have if we already had read the data.
> I'm not sure about this myself; it's much like the IOException that
> can be thrown from closing a Java stream. As far as I can see from [0],
> it would throw an error if it fails in reading the GIF terminating block
> or deallocating used memory.
>
>> But could you write this as
>>
>> if (DGifCloseFile(gif, NULL) == GIF_ERROR){
>>       return 0;
>> }
>>
> Done: http://cr.openjdk.java.net/~andrew/8081315/webrev.02/
>
>> ?
>>
>>
>> -phil.
>>
>>
>
> [0] http://giflib.sourceforge.net/gif_lib.html