Bug 28308 – allow down-sampling images during decoding

• Component: Images
• Status: RESOLVED FIXED

[Yong Li]

separated from bug 27561

[Yong Li]

Created attachment 34851 [details]
the patch

only JPEG and PNG decoders are modified to support this feature. Other decoders just ignore it.

[Adam Treat]

Comment on attachment 34851 [details]
the patch

Looking much, much better!  A few issues:

> +#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
> +        if (m_decoder) {
> +#   ifdef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS
> +            m_decoder->setMaxNumPixels(IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS);
> +#   else
> +            m_decoder->setMaxNumPixels(1024 * 1024);
> +#   endif
> +        }
> +#endif
> +    }

Shift this block down below to the next 'if (m_decoder)' and you eliminate an extra conditional and a nested one at that.  Also, there is weird indenting on the '#    ifdef' Please remove.

> +namespace {
> +	enum MatchType{
> +		Exact,
> +		UpperBound,
> +		LowerBound
> +	};
> +}

More weird indentation.

> +template <MatchType type> static int getScaledValue(const Vector<int>& scaledValues, int orig, int searchStart)
> +{
> +	int size = scaledValues.size();
> +    const int* dataStart = scaledValues.data();
> +    const int* dataEnd = dataStart + size;
> +    const int* pos = std::lower_bound(dataStart + searchStart, dataEnd, orig);
> +    switch (type) {
> +        case Exact:
> +            return pos != dataEnd && *pos == orig ? pos - dataStart : -1;
> +        case LowerBound:
> +            return pos != dataEnd && *pos == orig ? pos - dataStart : pos - dataStart - 1;
> +        case UpperBound:
> +        default:
> +            return pos != dataEnd ? pos - dataStart : -1;
> +    }
> +}

More weird indentation and the case labels should line up with switch statement. Please check with check-webkit-style.  Why is this templated?

> +int ImageDecoder::upperBoundScaledX(int origX, int searchStart)
> +{
> +	return getScaledValue<UpperBound>(m_scaledColumns, origX, searchStart);
> +}

Is this tabbed indentation? Please remove all tabs and check thoroughly for coding style.

> +        // ENABLE(IMAGE_DECODER_DOWN_SAMPLING) allows image decoders writedirectly to scaled output buffers

Typo. "allows image decoders to write directly to..."

> +        // by down sampling. Call setMaxNumPixels() to specify the biggest size that decoded images
> +        // can have. Image decoders will deflate those images that are bigger than m_maxNumPixels.
> +        // (Not supported by all image decoders yet)

> -static void convertCMYKToRGBA(RGBA32Buffer& dest, JSAMPROW src, jpeg_decompress_struct* info)
> +static void convertCMYKToRGBA(RGBA32Buffer& dest, int destY, JSAMPROW src, int srcWidth
> +#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
> +                              , bool scaled, const Vector<int>& scaledColumns
> +#endif
> +                              )
>  {
> -    ASSERT(info->out_color_space == JCS_CMYK);

Why do you remove this assert and change the behavior of this method even when IMAGE_DECODER_DOWN_SAMPLING is disabled?

> @@ -491,21 +507,33 @@ static void convertCMYKToRGBA(RGBA32Buffer& dest, JSAMPROW src, jpeg_decompress_
>  
>          // read_scanlines has increased the scanline counter, so we
>          // actually mean the previous one.
> -        dest.setRGBA(x, info->output_scanline - 1, c * k / 255, m * k / 255, y * k / 255, 0xFF);
> +        dest.setRGBA(x, destY, c * k / 255, m * k / 255, y * k / 255, 0xFF);

Same question as above and repeated for hunks below.

>      while (info->output_scanline < info->output_height) {
>          /* Request one scanline.  Returns 0 or 1 scanlines. */
> -        if (jpeg_read_scanlines(info, samples, 1) != 1)
> +        int sourceY = info->output_scanline;
> +        int sourceRows = jpeg_read_scanlines(info, samples, 1);
> +        if (sourceRows != 1) {
> +            if (sourceRows != 0)
> +                m_failed = true;
>              return false;
> +        }

Another change that is not guarded by ENABLE(IMAGE_DECODER_DOWN_SAMPLING)

Each of these changes should be described.  I simply don't have context to understand them.

> +#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
> +        bool setSize(int width, int height) {

Brace goes on next line.

I'll stop there and say that this still needs coding style cleanups.  And if at all possible the patch should have no behavior changes for when ENABLE(IMAGE_DECODER_DOWN_SAMPLING) is false.  Any other changes should either have some explanation or broken out into separate patch.

[Yong Li]

(In reply to comment #2)
> (From update of attachment 34851 [details])
> Looking much, much better!  A few issues:
 
> Shift this block down below to the next 'if (m_decoder)' and you eliminate an
> extra conditional and a nested one at that.

They are different. this 'if (m_decoder)' is reached only for the first time, where the below one is reached every time.

> Why is this templated?
To be faster. template argument is kind of const values.
> 
> Why do you remove this assert and change the behavior of this method even when
> IMAGE_DECODER_DOWN_SAMPLING is disabled?

info becomes useless except in this ASSERT. I removed 'info' argument. ASSERT is not necessary because the caller takes care of that.

> 
> >      while (info->output_scanline < info->output_height) {
> >          /* Request one scanline.  Returns 0 or 1 scanlines. */
> > -        if (jpeg_read_scanlines(info, samples, 1) != 1)
> > +        int sourceY = info->output_scanline;
> > +        int sourceRows = jpeg_read_scanlines(info, samples, 1);
> > +        if (sourceRows != 1) {
> > +            if (sourceRows != 0)
> > +                m_failed = true;
> >              return false;
> > +        }
> 
> Another change that is not guarded by ENABLE(IMAGE_DECODER_DOWN_SAMPLING)

I can remove the sourceRows change. sourceY is used to record the current row. With this change, there can be more code shared.

[Adam Treat]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 34851 [details] [details])
> > Looking much, much better!  A few issues:
> 
> > Shift this block down below to the next 'if (m_decoder)' and you eliminate an
> > extra conditional and a nested one at that.
> 
> They are different. this 'if (m_decoder)' is reached only for the first time,
> where the below one is reached every time.

Ahh, the wonders of reading diffs.  Sorry!

> > Why is this templated?
> To be faster. template argument is kind of const values.

Ok.

> > Why do you remove this assert and change the behavior of this method even when
> > IMAGE_DECODER_DOWN_SAMPLING is disabled?
> 
> info becomes useless except in this ASSERT. I removed 'info' argument. ASSERT
> is not necessary because the caller takes care of that.

Ok, I didn't see the added ASSERT.
 
> > Another change that is not guarded by ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
> 
> I can remove the sourceRows change. 

Thanks.

[Yong Li]

Created attachment 34867 [details]
modified for coding style...

[Eric Seidel (no email)]

Comment on attachment 34867 [details]
modified for coding style...

 126 #ifdef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS
 127             m_decoder->setMaxNumPixels(IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS);
 128 #else
 129             m_decoder->setMaxNumPixels(1024 * 1024);
 130 #endif

Would be better to check #ifdef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS at the top fo the file, and define IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS to 1024 * 1024 if not defined.

Seems we'd benifit from some nicely named local variables:
47     case Exact:
 48         return pos != dataEnd && *pos == orig ? pos - dataStart : -1;
 49     case LowerBound:
 50         return pos != dataEnd && *pos == orig ? pos - dataStart : pos - dataStart - 1;
 51     case UpperBound:
 52     default:
 53         return pos != dataEnd ? pos - dataStart : -1;

Style:
 84     double zoom = 1. /shrink;

I would have made this a static inline function to prevent copy/paste:
94     m_scaledRows.reserveCapacity(height * shrink + 0.5);
 95     for (int scaledY = 0;;) {
 96         int y = scaledY * zoom + 0.5;
 97         if (y < height) {
 98             m_scaledRows.append(y);
 99             ++scaledY;
 100         } else
 101             break;
 102     }

 64             bool good = ImageDecoder::setSize(width, height);

good is not the word you want.  success would be better.  setSizeOK would be another option.

[Yong Li]

(In reply to comment #6)
> (From update of attachment 34867 [details])
>  126 #ifdef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS
>  127            
> m_decoder->setMaxNumPixels(IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS);
>  128 #else
>  129             m_decoder->setMaxNumPixels(1024 * 1024);
>  130 #endif
> 
> Would be better to check #ifdef
> IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS at the top fo the file, and
> define IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS to 1024 * 1024 if not
> defined.
> 
> Seems we'd benifit from some nicely named local variables:
> 47     case Exact:
>  48         return pos != dataEnd && *pos == orig ? pos - dataStart : -1;
>  49     case LowerBound:
>  50         return pos != dataEnd && *pos == orig ? pos - dataStart : pos -
> dataStart - 1;
>  51     case UpperBound:
>  52     default:
>  53         return pos != dataEnd ? pos - dataStart : -1;
> 
> Style:
>  84     double zoom = 1. /shrink;
> 
> I would have made this a static inline function to prevent copy/paste:

for this 1./x ? I think that's sick

> 94     m_scaledRows.reserveCapacity(height * shrink + 0.5);
>  95     for (int scaledY = 0;;) {
>  96         int y = scaledY * zoom + 0.5;
>  97         if (y < height) {
>  98             m_scaledRows.append(y);
>  99             ++scaledY;
>  100         } else
>  101             break;
>  102     }
> 
>  64             bool good = ImageDecoder::setSize(width, height);
> 
> good is not the word you want.  success would be better.  setSizeOK would be
> another option.

"good" means the size is good.

r- with all these tiny things?

BTW, after it's modified to be what you want above, are you going to accept it?

[Yong Li]

Created attachment 34884 [details]
modified as Eric Seidel requires

[Adam Treat]

Comment on attachment 34884 [details]
modified as Eric Seidel requires

This looks good, but needs a slight clarification on landing.  Basically, please initialize sourceY after the call to 'jpeg_read_scanlines' to 'int sourceY = info->output_scanline - 1;' to emphasize that no behavior change has occurred where the 'IMAGE_DECODER_DOWN_SAMPLING' is not enabled.  Also add appropriate comment.  r+ with those changes.  Thanks!

[Yong Li]

landed @ r47371

[Gustavo Noronha (kov)]

Reopenning, since I reverted the commit, because it broke the GTK+ build.

[Yong Li]

47381 landed again

[Peter Kasting]

Created attachment 38751 [details]
Fix warnings

The patch landed on here caused signed vs. unsigned warnings on GTK.  This fixes them.  Someone please rubber-stamp.

[Jan Alonzo]

Comment on attachment 38751 [details]
Fix warnings

rs=me.

[Peter Kasting]

Comment on attachment 38751 [details]
Fix warnings

Landed in r47876.