Bug 85268 - PNGImageDecoder: Add ENABLE(IMAGE_DECODER_DOWN_SAMPLING) guards to rowAvailable
Summary: PNGImageDecoder: Add ENABLE(IMAGE_DECODER_DOWN_SAMPLING) guards to rowAvailable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-01 00:26 PDT by noel gordon
Modified: 2012-05-01 19:51 PDT (History)
2 users (show)

See Also:

Attachments
Patch (3.36 KB, patch)
2012-05-01 00:28 PDT, noel gordon 		no flags Details | Formatted Diff | Diff
Patch for landing (2.98 KB, patch)
2012-05-01 16:22 PDT, noel gordon 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2012-05-01 00:26:47 PDT 
PNGImageDecoder: Add ENABLE(IMAGE_DECODER_DOWN_SAMPLING) guards to rowAvailable
Comment 1 noel gordon 2012-05-01 00:28:55 PDT 
Created attachment 139598 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-01 08:24:02 PDT 
Comment on attachment 139598 [details]
Patch

I would have reversed that if I think, to avoid the !, but this is fine.
Comment 3 Eric Seidel (no email) 2012-05-01 08:24:58 PDT 
Comment on attachment 139598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139598&action=review

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:441
>          png_bytep pixel = row + (m_scaled ? m_scaledColumns[x] : x) * colorChannels;

Couldn't we just put the #if around this line?
Comment 4 noel gordon 2012-05-01 16:20:02 PDT 
(In reply to comment #2)
> (From update of attachment 139598 [details])
> I would have reversed that if I think, to avoid the !, but this is fine.

Reversed is better, so will do.

(In reply to comment #3)
> > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:441
> >          png_bytep pixel = row + (m_scaled ? m_scaledColumns[x] : x) * colorChannels;
> 
> Couldn't we just put the #if around this line?

Yes, but created more #if's nearby when I fiddled with it :(
Comment 5 noel gordon 2012-05-01 16:22:33 PDT 
Created attachment 139702 [details]
Patch for landing
Comment 6 Eric Seidel (no email) 2012-05-01 16:25:10 PDT 
Comment on attachment 139702 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=139702&action=review

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:438
> +    ASSERT(!m_scaled);

Why don't we just branch the whole for here, instead of having this ENABLE?  Why would someone want to disable this support?
Comment 7 noel gordon 2012-05-01 16:52:45 PDT 
(In reply to comment #6)
> (From update of attachment 139702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139702&action=review
> 
> > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:438
> > +    ASSERT(!m_scaled);
> 
> Why don't we just branch the whole for here, instead of having this ENABLE?  Why would someone want to disable this support?

Support is only enabled for Touch Mobile folks (WinCE port).  Could branch yes.  I'm really thinking of someone new to this code (tpayne) working here to add color correction support (this is the place to do it) and preempting their questions about image down scaling ...
Comment 8 WebKit Review Bot 2012-05-01 19:51:47 PDT 
Comment on attachment 139702 [details]
Patch for landing

Clearing flags on attachment: 139702

Committed r115782: <http://trac.webkit.org/changeset/115782>
Comment 9 WebKit Review Bot 2012-05-01 19:51:52 PDT 
All reviewed patches have been landed.  Closing bug.