[Webkit-unassigned] [Bug 28308] allow down-sampling images during decoding

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28308


Adam Treat <treat at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34851|review?                     |review-
               Flag|                            |




--- Comment #2 from Adam Treat <treat at kde.org>  2009-08-14 10:51:26 PDT ---
(From update of attachment 34851)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.