Last Comment Bug 418633 - imgITools fails to decodeImageData for GIF images
: imgITools fails to decodeImageData for GIF images
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla10
Assigned To: Fredrik Larsson (:nossralf)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-20 09:28 PST by Fredrik Larsson (:nossralf)
Modified: 2011-10-16 10:43 PDT (History)
6 users (show)
emorley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
gif test image, 32x32, 1809 bytes (1.77 KB, image/gif)
2008-02-20 13:16 PST, Fredrik Larsson (:nossralf)
no flags Details
unit test for mime type image/gif (1.42 KB, patch)
2008-02-20 13:20 PST, Fredrik Larsson (:nossralf)
joe: review+
Details | Diff | Splinter Review
Rebased to tip + with binary added (3.91 KB, patch)
2011-10-15 07:14 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Rebased + binary + remove TESTDIR param (3.90 KB, patch)
2011-10-15 09:16 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review

Description Fredrik Larsson (:nossralf) 2008-02-20 09:28:27 PST
When trying to use the image tools to decode a GIF image, the decodeImageData function throws NS_ERROR_NOT_IMPLEMENTED.

The reason is imgTools::DecodeImageData always [1] calls the Flush method for the image decoder, which in turn isn't implemented by nsGIFDecoder2 [2]. All other image decoders return NS_OK for Flush, even if it's a no-op.

The last patch for bug 399925 changed the gif decoder's Flush method to return NS_OK, but it was backed out, so this problem still exists.

[1] http://mxr.mozilla.org/mozilla/source/modules/libpr0n/src/imgTools.cpp#238
[2] http://mxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp#177

I also notice there is no unit test to cover image/gif for imgITools, and I would gladly write one if the powers that be wish me to do so.
Comment 1 Matthew Gertner 2008-02-20 09:39:11 PST
Writing a unit test for GIF sounds like a great idea to me.
Comment 2 Fredrik Larsson (:nossralf) 2008-02-20 13:16:00 PST
Created attachment 304581 [details]
gif test image, 32x32, 1809 bytes

A possible GIF test image: converted image2jpg32x32.png to GIF using Gimp, named it image4.gif.
Comment 3 Fredrik Larsson (:nossralf) 2008-02-20 13:20:15 PST
Created attachment 304583 [details] [diff] [review]
unit test for mime type image/gif

Tentative unit test addition for test_imgtools.js.
Comment 4 Joe Drew (not getting mail) 2011-10-12 12:04:46 PDT
Comment on attachment 304583 [details] [diff] [review]
unit test for mime type image/gif

Review of attachment 304583 [details] [diff] [review]:
-----------------------------------------------------------------

Looks lovely. This patch might need some massaging to commit though - at least the gif (the other attachment) is going to have to be hg added.
Comment 5 Ed Morley [:emorley] 2011-10-15 07:14:05 PDT
Created attachment 567281 [details] [diff] [review]
Rebased to tip + with binary added
Comment 6 Ed Morley [:emorley] 2011-10-15 07:20:21 PDT
In my queue with a few other bits that are being sent to try first and then onto inbound :-)

https://tbpl.mozilla.org/?tree=Try&rev=488d4a5f274a
Comment 7 Ed Morley [:emorley] 2011-10-15 09:15:00 PDT
Failed xpcshell due to:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | FAILED in test #10 -- test decoding a GIF: ReferenceError: TESTDIR is not defined

This:
> imgFile = do_get_file(TESTDIR + imgName);
should presumably be:
> imgFile = do_get_file(imgName);
Comment 8 Ed Morley [:emorley] 2011-10-15 09:16:08 PDT
Created attachment 567287 [details] [diff] [review]
Rebased + binary + remove TESTDIR param
Comment 9 Ed Morley [:emorley] 2011-10-15 09:17:07 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c81b8943dad6

Note You need to log in before you can comment on or make changes to this bug.