Bug 100164 – Pass on exif orientation from ImageSource when using the open-source image decoders

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

Pass on exif orientation from ImageSource when using the open-source image decoders

[Nico Weber]

Created attachment 170248 [details]
Patch

[WebKit Review Bot]

Attachment 170248 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderObject.cpp:2225:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nico Weber]

+noel.gordon fyi

[noel gordon]

Comment on attachment 170248 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        default). However, the feature needs port-specific modifications to
> +        GraphicsContext (without this, webcore will use the rotated bounds but
> +        draw the image as if it hadn't be rotated, resulting in squished
> +        pixels), and most ports don't implement these yet -- so put
> +        turning this on for image documents behind a port-specific #ifdef.

Were bugs filed about this?  Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels.  Perhaps I'm not understanding.

> Source/WebCore/platform/graphics/ImageSource.cpp:110
>  }

Good.

> Source/WebCore/platform/graphics/ImageSource.cpp:122
> +    if (!m_decoder)
> +        return IntSize();
> +
> +    IntSize size = m_decoder->frameSizeAtIndex(index);
> +    if ((shouldRespectOrientation == RespectImageOrientation) && m_decoder->orientation().usesWidthAsHeight())
> +        return IntSize(size.height(), size.width());
>  
> -    return m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize();
> +    return size;
>  }

m_decoder->frameSizeAtIndex(index) could return an empty IntSize, right?  Maybe write this function body as:

IntSize size = m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize();

if (shouldRespectOrientation == RespectImageOrientation && orientationAtIndex(index).usesWidthAsHeight())
    return IntSize(size.height(), size.width());

return size;

> Source/WebCore/platform/graphics/ImageSource.cpp:184
> +    return m_decoder ? m_decoder->orientation() : ImageOrientation();

return m_decoder ? m_decoder->orientation() : DefaultImageOrientation;

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:156
> +    return m_actualDecoder ? m_actualDecoder->orientation() : ImageOrientation();

return m_actualDecoder ? m_actualDecoder->orientation() : DefaultImageOrientation;

You should prefix this statement with:

// FIXME: Make this work with deferred image decoding.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:182
> +    // The JPEG decoder looks at EXIF metadata.
> +    // TODO: Possibly implement XMP and IPTC support.

If you renamed the function to readExifOrientation, the first comment would be redundant :)  The rename could happen in another patch.  The second comment should be a FIXME, but I'm not sure this is a great place for it.  Drop it, or file a bug about it.

> Source/WebCore/rendering/RenderObject.cpp:2226
> +#endif

Use an if statement to avoid the style nit @2225

[Nico Weber]

Created attachment 170414 [details]
Patch

[Nico Weber]

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

>> Source/WebCore/ChangeLog:14
>> +        turning this on for image documents behind a port-specific #ifdef.
> 
> Were bugs filed about this?  Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels.  Perhaps I'm not understanding.

Before this CL, orientation was never set on images, so the bug wasn't visible. This CL keeps it that way. If you want to see the squishing, patch in this CL to trunk webkit, remove the ifdef, and look at a exif-rotated jpeg.

>> Source/WebCore/platform/graphics/ImageSource.cpp:122
>>  }
> 
> m_decoder->frameSizeAtIndex(index) could return an empty IntSize, right?  Maybe write this function body as:
> 
> IntSize size = m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize();
> 
> if (shouldRespectOrientation == RespectImageOrientation && orientationAtIndex(index).usesWidthAsHeight())
>     return IntSize(size.height(), size.width());
> 
> return size;

The current code is more similar to ImageSourceCG. Do you see a problem with the corrent code if frameSizeAtIndex() returns an empty size?

>> Source/WebCore/platform/graphics/ImageSource.cpp:184
>> +    return m_decoder ? m_decoder->orientation() : ImageOrientation();
> 
> return m_decoder ? m_decoder->orientation() : DefaultImageOrientation;

Done.

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:156
>> +    return m_actualDecoder ? m_actualDecoder->orientation() : ImageOrientation();
> 
> return m_actualDecoder ? m_actualDecoder->orientation() : DefaultImageOrientation;
> 
> You should prefix this statement with:
> 
> // FIXME: Make this work with deferred image decoding.

Done.

Re FIXME: Does it not already work? Why not?

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:182
>> +    // TODO: Possibly implement XMP and IPTC support.
> 
> If you renamed the function to readExifOrientation, the first comment would be redundant :)  The rename could happen in another patch.  The second comment should be a FIXME, but I'm not sure this is a great place for it.  Drop it, or file a bug about it.

Changed to FIXME

>> Source/WebCore/rendering/RenderObject.cpp:2226
>> +#endif
> 
> Use an if statement to avoid the style nit @2225

I think the style nit isn't critical.

[WebKit Review Bot]

Attachment 170414 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderObject.cpp:2225:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Looks fine to me, but Noel may have further comments.  I think SYD is asleep right now.

[Eric Seidel (no email)]

Comment on attachment 170414 [details]
Patch

r=me.  Please consult with Noel before landing.

[Nico Weber]

Created attachment 170506 [details]
Patch

[Nico Weber]

Created attachment 170510 [details]
Patch

[Nico Weber]

Created attachment 170511 [details]
Patch

[noel gordon]

(In reply to comment #6)
> View in context: https://bugs.webkit.org/attachment.cgi?id=170248&action=review
> 
> >> Source/WebCore/ChangeLog:14
> >> +        turning this on for image documents behind a port-specific #ifdef.
> > 
> > Were bugs filed about this?  Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels.  Perhaps I'm not understanding.
> 
> Before this CL, orientation was never set on images, so the bug wasn't visible. This CL keeps it that way. If you want to see the squishing, patch in this CL to trunk webkit, remove the ifdef, and look at a exif-rotated jpeg.


We need BitmapImage::draw modifications whichj we're fixing elsewhere. 

> >> Source/WebCore/platform/graphics/ImageSource.cpp:122
> >>  }
> > 
> > m_decoder->frameSizeAtIndex(index) could return an empty IntSize, right?  Maybe write this function body as:
> > 
> > IntSize size = m_decoder ? m_decoder->frameSizeAtIndex(index) : IntSize();
> > 
> > if (shouldRespectOrientation == RespectImageOrientation && orientationAtIndex(index).usesWidthAsHeight())
> >     return IntSize(size.height(), size.width());
> > 
> > return size;
> 
> The current code is more similar to ImageSourceCG. Do you see a problem with the current code if frameSizeAtIndex() returns an empty size?

Nope.

 
> > // FIXME: Make this work with deferred image decoding.
> 
> Done.
> Re FIXME: Does it not already work? Why not?

Because m_actualDecoder becomes NULL when deferred decoding starts.

 
> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:182
> >> +    // TODO: Possibly implement XMP and IPTC support.
> > 
> > If you renamed the function to readExifOrientation, the first comment would be redundant :)  The rename could happen in another patch.  The second comment should be a FIXME, but I'm not sure this is a great place for it.  Drop it, or file a bug about it.
> 
> Changed to FIXME
> 
> >> Source/WebCore/rendering/RenderObject.cpp:2226
> >> +#endif
> > 
> > Use an if statement to avoid the style nit @2225
> 
> I think the style nit isn't critical.

Not critical but it makes noise for me and others.

I'm fine with this patch though.

[noel gordon]

Lots of mid-air collisions while trying to say the above :)

[WebKit Review Bot]

Comment on attachment 170511 [details]
Patch

Clearing flags on attachment: 170511

Committed r132433: <http://trac.webkit.org/changeset/132433>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #13)
> (In reply to comment #6)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170248&action=review
> > 
> > >> Source/WebCore/ChangeLog:14
> > >> +        turning this on for image documents behind a port-specific #ifdef.
> > > 
> > > Were bugs filed about this?  Chromium didn't have the port-specific modifications you mention, yet I don't recall any chromium bugs about squished pixels.  Perhaps I'm not understanding.
> > 
> > Before this CL, orientation was never set on images, so the bug wasn't visible. This CL keeps it that way. If you want to see the squishing, patch in this CL to trunk webkit, remove the ifdef, and look at a exif-rotated jpeg.
> 
> We need BitmapImage::draw modifications which we're fixing elsewhere. 

And to be precise, since ImageSource now returns orientation on all ports, those without the BitmapImage::draw modifications will squish.  Hence the #ifdef.

The #ifdef is missing an || PLATFORM(CHROMIUM) fyi, so imageDocument orientation is not yet working on chromium.