869723 - Add EXIF parsing to imagelib

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

We need to parse EXIF data (specifically, EXIF orientation data) to support the CSS3 image-orientation attribute. This bug is about getting code to do that job in the tree.

I've looked at a few options and I have to agree with bug 298619 comment 81 that EasyEXIF is the smoothest path forward:

http://code.google.com/p/easyexif/

It's got a BSD license, it should be simple to integrate, and it's actively maintained. It does more than we need; I don't think we need to both extracting the camera manufacturer's name as a string at this point, for example. However, the code is easy to read, and the structure of it is such that we can easily #ifdef out everything we don't need and get just the data we want. If we need more later, we can #ifdef it back.

Comment 1

[Joe Drew (not getting mail)]

Do we really need a full EXIF reader? Could we just make a cut-down EXIF orientation reader?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #1)
> Do we really need a full EXIF reader? Could we just make a cut-down EXIF
> orientation reader?

I would like this better. It will make it easier to audit the code.

Comment 3

[Seth Fowler [:seth] [:s2h]]

IMO using a library that others are using and that is externally supported is a substantial benefit in terms of using our resources more efficiently, so I'd like to stick with the approach of using EasyEXIF. I think we can get the best of both worlds here, though. EasyEXIF is an absurdly small library - there's not that much code there to start with - and it's structured in such a way that #ifdef'ing out the features we don't want is trivial. Note that the "interesting" code in the library is exactly the code we'd have to write anyway. It should still be easy to pull in updates from upstream as we need them, and the nice thing is that if we ever want some additional EXIF data (and it's not inconceivable that B2G might it at some point) it's just a quick #ifdef away.

Comment 4

[Fred Wenzel [:wenzel]]

I concur with Seth: To find the orientation we'll have to read the whole EXIF blog anyway (then discard everything that's not Orientation, or possibly just stop when we find it). By the time we've done that, we already have a full-blown EXIF library in the tree.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add the EasyEXIF library for parsing EXIF data.

This drops the EasyEXIF library in, with appropriate modifications for our needs.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

This adds a getter for an image's intrinsic orientation and an appropriate enum that reflects the values supported by CSS3.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Parse EXIF orientation in nsJPEGDecoder.

This patch makes us actually parse the EXIF orientation in nsJPEGDecoder once we have the image size available. (Since EXIF is specified to be the first chunk in the file it should always be available at this point if it's present at all.)

Comment 8

[Seth Fowler [:seth] [:s2h]]

BTW, just to save the reviewers some time: in bug 870090 the license for EasyEXIF got reviewed by gerv and landed in the tree, as required. So we're good to go on that front.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 756809 [details] [diff] [review]
(Part 1) - Add the EasyEXIF library for parsing EXIF data.

sr=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 756811 [details] [diff] [review]
(Part 2) - Add imgIContainer::GetOrientation.

>+  const unsigned short ORIENTATION_MAX_VALUE       = 7;

Why is this needed?  Please document or remove.

sr=me

Comment 11

[Fred Wenzel [:wenzel]]

Comment on attachment 756811 [details] [diff] [review]
(Part 2) - Add imgIContainer::GetOrientation.

>+  const unsigned short ORIENTATION_NORMAL          = 0;
>+  const unsigned short ORIENTATION_NORMAL_FLIP     = 1;
>+  const unsigned short ORIENTATION_ROTATE_90       = 2;
>+  const unsigned short ORIENTATION_ROTATE_90_FLIP  = 3;
>+  const unsigned short ORIENTATION_ROTATE_180      = 4;
>+  const unsigned short ORIENTATION_ROTATE_180_FLIP = 5;
>+  const unsigned short ORIENTATION_ROTATE_270      = 6;
>+  const unsigned short ORIENTATION_ROTATE_270_FLIP = 7;
>+  const unsigned short ORIENTATION_MAX_VALUE       = 7;

Are you certain these are accurate? I used this as a reference when dealing with orientation:
<http://sylvana.net/jpegcrop/exif_orientation.html>

Granted, it's a secondary source, so perhaps inaccurate? At any rate theirs isn't 0-indexed, and also the highest value (your "7") corresponds to "90-degrees clockwise, no flip". How come the discrepancy?

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Fred Wenzel [:wenzel] from comment #11)
> Granted, it's a secondary source, so perhaps inaccurate? At any rate theirs
> isn't 0-indexed, and also the highest value (your "7") corresponds to
> "90-degrees clockwise, no flip". How come the discrepancy?

I arranged these values in a way that made sense to me and matches the conventions of our code, rather than in the way that EXIF arranges them. There's no particular reason to stick to EXIF's ordering for our internal use. EXIF may not even be the only source for this information at some point.

Comment 13

[Fred Wenzel [:wenzel]]

Makes sense, thanks

Comment 14

[Seth Fowler [:seth] [:s2h]]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Comment on attachment 756811 [details] [diff] [review]
> (Part 2) - Add imgIContainer::GetOrientation.
> 
> >+  const unsigned short ORIENTATION_MAX_VALUE       = 7;
> 
> Why is this needed?  Please document or remove.
> 
> sr=me

Thanks for the review, Boris!

ORIENTATION_MAX_VALUE is used for error checking in an identical way to FRAME_MAX_VALUE. I'll add documentation about it.

Comment 15

[Joe Drew (not getting mail)]

Comment on attachment 756809 [details] [diff] [review]
(Part 1) - Add the EasyEXIF library for parsing EXIF data.

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

What I'd like us to do, instead, is see if we can get upstream an "only support orientation" mode for EasyEXIF, so we don't have to interleave #if 0 throughout.

However, in order to review this, I'd like to have a "semi-preprocessed" version of it, with all the #if 0'd bits stripped out, so I can see what we're actually going to be shipping.

If that looks small enough, I may want us to switch to using that; I'll reserve judgement until then :)

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 756811 [details] [diff] [review]
(Part 2) - Add imgIContainer::GetOrientation.

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

::: image/public/imgIContainer.idl
@@ +311,5 @@
> +  /*
> +   * Returns the inherent orientation of the image, as described in the image's
> +   * metadata (e.g. EXIF).
> +   */
> +  [notxpcom] unsigned short getOrientation();

Is there any reason to expose this to anyone else? Could we just add a draw flag "OBEY_ORIENTATION" or whatever?

::: image/src/ImageMetadata.h
@@ +63,5 @@
>    // The loop count for animated images, or -1 for infinite loop.
>    int32_t mLoopCount;
>  
>    Maybe<nsIntSize> mSize;
> +  Maybe<uint16_t>  mOrientation;

I'm uncertain about storing this as a magic value. Would it be cleaner if we had a 2D matrix, or a pair: (angle, flip boolean)?

::: image/src/RasterImage.h
@@ +190,5 @@
>    /**
>     * Sets the size of the container. This should only be called by the
>     * decoder. This function may be called multiple times, but will throw an
>     * error if subsequent calls do not match the first.
>     */

Comment should be updated

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 756814 [details] [diff] [review]
(Part 3) - Parse EXIF orientation in nsJPEGDecoder.

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

::: image/decoders/nsJPEGDecoder.cpp
@@ +542,5 @@
> +
> +  // Walk the marker list and try to locate the EXIF data.
> +  for (marker = mInfo.marker_list ; marker != nullptr ; marker = marker->next) {
> +    // EXIF uses the APP1 marker.
> +    if (marker->marker == JPEG_APP0 + 1) 

whitespace

@@ +562,5 @@
> +      case 4: return imgIContainer::ORIENTATION_ROTATE_180_FLIP;
> +      case 5: return imgIContainer::ORIENTATION_ROTATE_270_FLIP;
> +      case 6: return imgIContainer::ORIENTATION_ROTATE_90;
> +      case 7: return imgIContainer::ORIENTATION_ROTATE_90_FLIP;
> +      case 8: return imgIContainer::ORIENTATION_ROTATE_270;

These should all have the return on its own line.

Comment 18

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #15)
> Comment on attachment 756809 [details] [diff] [review]
> (Part 1) - Add the EasyEXIF library for parsing EXIF data.
> 
> Review of attachment 756809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What I'd like us to do, instead, is see if we can get upstream an "only
> support orientation" mode for EasyEXIF, so we don't have to interleave #if 0
> throughout.
> 
> However, in order to review this, I'd like to have a "semi-preprocessed"
> version of it, with all the #if 0'd bits stripped out, so I can see what
> we're actually going to be shipping.
> 
> If that looks small enough, I may want us to switch to using that; I'll
> reserve judgement until then :)

I'll make a version with the #if 0 stuff ripped out. The reason I did it this way is that it will be simpler to update our version of the library when the upstream version changes, since git/hg's merge algorithm can handle this sort of thing pretty well. It's not such a large piece of code that backporting fixes is likely to be a major issue in any case, though, so it might be fine to just leave it ripped out.

Comment 19

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #16)
> Is there any reason to expose this to anyone else? Could we just add a draw
> flag "OBEY_ORIENTATION" or whatever?

It's not enough to just change Draw; things like the GetWidth, GetHeight, and GetIntrinsicRatio have to change as well. IMO this will be simpler if layout checks the CSS applying to the image and uses an ImageWrapper to apply the orientation change if appropriate. We will need that anyway since CSS can force an orientation even if the image doesn't specify one.

> I'm uncertain about storing this as a magic value. Would it be cleaner if we
> had a 2D matrix, or a pair: (angle, flip boolean)?

I'd prefer to use a type which can take on only the options which are valid. (angle, flip boolean) could work fine if angle is an enumeration. Since generic pairs are kinda ugly in C++, though, I think it'd be better to define a struct to hold this stuff. That does sound cleaner, so I'll make that change.

Thanks for the review!

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (FOR REVIEW ONLY - VERSION WITH #if 0 BLOCKS REMOVED) - Add the EasyEXIF library for parsing EXIF data.

Finally popped my stack enough to circle back around to this. Joe, this is a version of part 1 with the #if 0 blocks stripped out, as you requested for review purposes.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add the EasyEXIF library for parsing EXIF data.

Minor change to avoid doing any work at all for e.g. string values.

I've contacted the author and we've chatted about upstreaming the most important change here, making it possible to avoid scanning for the EXIF header for cases like ours where we've already pulled out the APP1 segment using libjpeg. He said that'd be fine, so we'll be able to get that upstreamed.

I'm skeptical about upstreaming the other changes, where we rip out a lot of the functionality. I can't really envision an API to make this 'clean' that doesn't significantly complicate what is intended to be a pretty simple library. I think the best bet there is just to keep ripping that stuff out in our own local copy.

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

All of the review comments so far should be addressed. The most important change is that an Orientation type has been added to replace the constants that existed before.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Parse EXIF orientation in nsJPEGDecoder.

Addressed review comments and updated to use the new Orientation type.

Comment 24

[Seth Fowler [:seth] [:s2h]]

Attachment: (FOR REVIEW ONLY - VERSION WITH #if 0 BLOCKS REMOVED) - Add the EasyEXIF library for parsing EXIF data.

Updated to incorporate the minor changes to part 1 above.

Comment 25

[Joe Drew (not getting mail)]

Comment on attachment 779526 [details] [diff] [review]
(FOR REVIEW ONLY - VERSION WITH #if 0 BLOCKS REMOVED) - Add the EasyEXIF library for parsing EXIF data.

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

As it is, we can't ship exif.cpp. It's filled with assumptions about the buffer size that we won't be able to guarantee. It needs a hardening, and then a security review.

I think it might be worthwhile for us to write an orientation-only EXIF parser from scratch. I'm not convinced there's much to salvage here.

::: media/easyexif/exif.cpp
@@ +77,5 @@
> +    if (num_components <= 4)
> +      value.assign( (const char*)&data, num_components );
> +    else {
> +      if (base+data+num_components <= len)
> +        value.assign( (const char*)(buf+base+data), num_components );

This code is crazy, but at least parseEXIFString does not appear to be used. (Crazy in that it reads from data in one branch and buf + base + data in another)

@@ +131,5 @@
> +  unsigned offs   = 0;        // current offset into buffer
> +  if (!buf || len == 0)
> +    return PARSE_EXIF_ERROR_NO_EXIF;
> +  if (!std::equal(buf+offs, buf+offs+6, "Exif\0\0"))
> +    return PARSE_EXIF_ERROR_NO_EXIF;

At minimum we have to check for whether len is <= 6

@@ +142,5 @@
> +  // the global offset counter. No need for bounds checking here because
> +  // we checked in the previous section.
> +  unsigned tiff_header_start = offs;
> +  if (buf[offs] == 'I' && buf[offs+1] == 'I')
> +    alignIntel = true;

This doesn't check for length at all

@@ +155,5 @@
> +  if (0x2a != parse16(buf+offs, alignIntel))
> +    return PARSE_EXIF_ERROR_CORRUPT;
> +  offs += 2;
> +  unsigned first_ifd_offset = parse32(buf + offs, alignIntel);
> +  offs += first_ifd_offset - 4;

neither does all of this

@@ +168,5 @@
> +  // bytes of data.
> +  if (offs + 2 > len)
> +    return PARSE_EXIF_ERROR_CORRUPT;
> +  int num_entries = parse16(buf + offs, alignIntel);
> +  if (offs + 6 + 12 * num_entries > len)

All these magic numbers really don't fill me with confidence

Comment 26

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #25)
> Comment on attachment 779526 [details] [diff] [review]
> (FOR REVIEW ONLY - VERSION WITH #if 0 BLOCKS REMOVED) - Add the EasyEXIF
> library for parsing EXIF data.
> 
> Review of attachment 779526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As it is, we can't ship exif.cpp. It's filled with assumptions about the
> buffer size that we won't be able to guarantee. It needs a hardening, and
> then a security review.

Heh, alright, I'll give this some thought and decide what to do. I'll circle back around once I have the layout and CSS stuff in good shape.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

Add some convenience methods to Orientation and switch the enums to use the facilities in TypedEnum.h.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Parse EXIF orientation in nsJPEGDecoder.

Update to use the new TypedEnum-based enums.

Comment 29

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add OrientedImage.

This is the remaining major piece of the puzzle (at least, at the imagelib layer). This patch adds an Image wrapper called OrientedImage, exposed through ImageOps for layout to use. It applies to an image the rotation and/or reflection specified by an Orientation.

Comment 30

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Parse EXIF orientation in nsJPEGDecoder.

While writing reftests I noticed that I reversed two of the EXIF orientations by mistake. After this version they are definitely all correct; I've checked them against another implementation.

Comment 31

[Joe Drew (not getting mail)]

Comment on attachment 781487 [details] [diff] [review]
(Part 4) - Add OrientedImage.

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

Would've helped to have reviewed attachment 781456 [details] [diff] [review] first :)

We're going to want tests files for all these combinations (flip and angle).

I'm r+ with some concerns below that need to be addressed.

::: image/src/ImageOps.cpp
@@ +54,5 @@
> +
> +/* static */ already_AddRefed<imgIContainer>
> +ImageOps::Orient(imgIContainer* aImage, Orientation aOrientation)
> +{
> +  nsCOMPtr<imgIContainer> orientedImage =

this could just be an nsRefPtr too. Though I guess you used nsCOMPtr everywhere else in the imgIContainer paths of this file. :)

::: image/src/OrientedImage.cpp
@@ +56,5 @@
> +  if (mOrientation.SwapsWidthAndHeight()) {
> +    nsSize innerSize;
> +    if (NS_SUCCEEDED(rv = InnerImage()->GetIntrinsicSize(&innerSize))) {
> +      aSize->width = innerSize.height;
> +      aSize->height = innerSize.width;

You could maybe do the same in both cases, and if SwapsWidthAndHeight, std::swap() those two. Whatever you like.

@@ +74,5 @@
> +  if (mOrientation.SwapsWidthAndHeight()) {
> +    nsSize innerRatio;
> +    if (NS_SUCCEEDED(rv = InnerImage()->GetIntrinsicRatio(&innerRatio))) {
> +      aRatio->width = innerRatio.height;
> +      aRatio->height = innerRatio.width;

But if you do, do it here too :)

@@ +104,5 @@
> +    rv1 = GetWidth(&width);
> +    rv2 = GetHeight(&height);
> +  }
> +  NS_ENSURE_SUCCESS(rv1, rv1);
> +  NS_ENSURE_SUCCESS(rv2, rv2);

maybe have a single rv, and |= it? And don't NS_ENSURE_SUCCESS; hiding NS_WARNING and return is poops.

@@ +110,5 @@
> +  // Create a surface to draw into.
> +  mozilla::RefPtr<mozilla::gfx::DrawTarget> target;
> +  target = gfxPlatform::GetPlatform()->
> +    CreateOffscreenDrawTarget(gfx::IntSize(width, height),
> +                              gfx::FORMAT_B8G8R8A8);

We should only use alpha here (and in DrawPixelSnapped, below) if we actually have alpha.

@@ +125,5 @@
> +  // Draw.
> +  nsRefPtr<gfxContext> ctx = new gfxContext(surface);
> +  gfxRect imageRect(0, 0, width, height);
> +  gfxUtils::DrawPixelSnapped(ctx, drawable, OrientationMatrix(nsIntSize(width, height)),
> +                             imageRect, imageRect, imageRect, imageRect,

lol

@@ +163,5 @@
> +    } else {
> +      nsresult rv1 = GetWidth(&width);
> +      nsresult rv2 = GetHeight(&height);
> +      if (NS_FAILED(rv1) || NS_FAILED(rv2)) {
> +        // Fall back to identity if the width and height aren't available.

here you can probably |= the rv instead of using two, if you like.

@@ +177,5 @@
> +        break;
> +      case Flip::Horizontal:
> +        if (mOrientation.SwapsWidthAndHeight()) {
> +            matrix.Translate(gfxPoint(0, width));
> +            matrix.Scale(1.0, -1.0);

Shouldn't you handle this as part of the rotation instead?

@@ +205,5 @@
> +        matrix.Translate(gfxPoint(height, 0));
> +        matrix.Rotate(-1.5 * M_PI);
> +        break;
> +      default:
> +        MOZ_ASSERT(false, "Invalid rotation value");

I'm a little concerned by the the concatenation of flips and rotations here. At least in my noodling around in trying to verify/prove it wrong, it seems possible to produce nonsense results, i.e. combine a horizontal flip with a 90 degree rotation and you map (1, 1) to (w-1, w-1). Maybe I'm misinterpreting these results, though. A comprehensive test suite would help.

@@ +235,5 @@
> +
> +  // Update the subimage.
> +  gfxRect gfxSubimage(
> +      matrix.TransformBounds(
> +        gfxRect(aSubimage.x, aSubimage.y, aSubimage.width, aSubimage.height)));

Just put this on one line; >80 characters is a small price to pay for non-ugly code.

@@ +243,5 @@
> +  // since it's only a size a swap is enough.)
> +  nsIntSize viewportSize(aViewportSize);
> +  if (mOrientation.SwapsWidthAndHeight()) {
> +    viewportSize.width = aViewportSize.height;
> +    viewportSize.height = aViewportSize.width;

std::swap

@@ +245,5 @@
> +  if (mOrientation.SwapsWidthAndHeight()) {
> +    viewportSize.width = aViewportSize.height;
> +    viewportSize.height = aViewportSize.width;
> +  }
> +  

whitespace

::: image/src/OrientedImage.h
@@ +45,5 @@
> +                  const nsIntSize& aViewportSize,
> +                  const SVGImageContext* aSVGContext,
> +                  uint32_t aWhichFrame,
> +                  uint32_t aFlags) MOZ_OVERRIDE;
> + 

whitespace

::: image/src/imgFrame.cpp
@@ +467,5 @@
>      return;
>    }
>  
>    gfxMatrix userSpaceToImageSpace = aUserSpaceToImageSpace;
> +  gfxRect sourceRect = userSpaceToImageSpace.TransformBounds(aFill);

This change is needed because userSpaceToImageSpace now regularly contains translations, right?

Comment 32

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add OrientedImage.

This addresses the review concerns, but see the following comment.

Comment 33

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Joe! Lots of good comments!

(In reply to Joe Drew (:JOEDREW! \o/) from comment #31)

> Would've helped to have reviewed attachment 781456 [details] [diff] [review]
> first :)

This is gonna be a mindblower: you did! =) Perhaps it was an accidental r+?
 
> We're going to want tests files for all these combinations (flip and angle).

There is a big and growing suite of tests in bug 825771. It's easier to test there as that's where I add the CSS and layout code to interface with this stuff.

> std::swap() those two.

Yes! Good call; made things much cleaner.

> maybe have a single rv, and |= it?

You don't seem to be able to combine nsresults that way, but I switch to combining them manually with the ternary operator, which has the nice advantage of short circuiting.

> We should only use alpha here (and in DrawPixelSnapped, below) if we
> actually have alpha.

I tried to do this. Is gfx::FORMAT_B8G8R8X8 the correct format when there's no alpha?

> Shouldn't you handle this [the flip] as part of the rotation instead?

I don't see how I could; maybe I misunderstand what you're saying here.

> I'm a little concerned by the the concatenation of flips and rotations here.

The test suite (and concerns caused by your comment) have definitely led me to flush out some bugs. I think things are all OK now, though.


> This change [TransformBounds] is needed because userSpaceToImageSpace now regularly contains
> translations, right?

The need for it was uncovered because userSpaceToImageSpace now regularly contains rotations, but it's the correct thing anyway. It's what we do elsewhere, for example in DrawPixelSnapped.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add OrientedImage.

Remove a small piece of debug code that got left in.

Comment 35

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #33)
> > We should only use alpha here (and in DrawPixelSnapped, below) if we
> > actually have alpha.
> 
> I tried to do this. Is gfx::FORMAT_B8G8R8X8 the correct format when there's
> no alpha?

Yep!

Comment 36

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add an EXIF parser to imagelib.

OK, based on the preceding discussion in this bug I went ahead and implemented an EXIF parser. This completely replaces EasyEXIF; I'll add another patch later that will remove EasyEXIF's license from the license file. The parser is specialized to only handle orientation at the moment, although I tried to write it in such a way that extending it in the future won't be hard.

Comment 37

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Parse EXIF orientation in nsJPEGDecoder.

This is a rebase of this patch against the new part 1. Nothing of substance has changed, so this doesn't require rereview.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 786675 [details] [diff] [review]
(Part 1) - Add an EXIF parser to imagelib.

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

Some suggestions on the byte stream reading based on what qcms (iccread.c) does:
- Don't have the current location as class state, pass this around explicitly (readInt takes an offset parameter) I find this makes it clearer in the caller what location it's reading from and makes thing feel a bit more 'functional'
- The readInt type functions return 0 and mark the the input data as bad if reading beyond the bounds. This avoids having to error check every readInt call.

Comment 39

[Seth Fowler [:seth] [:s2h]]

Thanks for the feedback, Jeff!

(In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> - Don't have the current location as class state, pass this around
> explicitly (readInt takes an offset parameter) I find this makes it clearer
> in the caller what location it's reading from and makes thing feel a bit
> more 'functional'

Yeah, I like that. I'll make that change.

> - The readInt type functions return 0 and mark the the input data as bad if
> reading beyond the bounds. This avoids having to error check every readInt
> call.

I'm on the fence about this one. This doesn't work very cleanly with alternatives (although, admittedly, there's only one case of that in the subset of the EXIF grammar I've implemented) and my existing approach can be made _much_ more concise trivially by using short-circuiting boolean operators. (In fact, that's normally the approach I use in C++, but I am a bit more conservative about style in Gecko code than in code I write for other projects.) For processing large amounts of data, there can be a performance advantage to deferring error detection, but I think there is not much real world advantage either way here.

I'll update the patch to incorporate the offset parameter suggestion and make a style change that I think will make the boolean return value approach more aesthetically pleasing.

Comment 40

[Seth Fowler [:seth] [:s2h]]

Attachment: (TEST) - Functional version of parser. (Do not review or checkin.)

Well, I went ahead and created a purely functional version of the parser.

TBH, I'm not sure it's an improvement. For now, I'm leaning towards sticking with the original version.

Comment 41

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

Rebase of part 2 against current m-c.

Comment 42

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

Switch StandardInteger.h for stdint.h to fix build issue.

Comment 43

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add an EXIF parser to imagelib.

Again, replace StandardInteger.h with stdint.h.

Comment 44

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

Not sure if I uploaded the wrong version of the patch or what but cdiehl reports patch apply failures with the version up here now, which I definitely don't get locally, so reuploading.

Comment 45

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add imgIContainer::GetOrientation.

Fix C++03-incompatible trailing commas.

Comment 46

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add an EXIF parser to imagelib.

Fix another trailing comma issue.

Comment 47

[Timothy Nikkel (:tnikkel)]

Comment on attachment 789939 [details] [diff] [review]
(Part 1) - Add an EXIF parser to imagelib.

I didn't read the spec, so this review is assuming a faithful implementation of the spec.

Can we use an explicit type for length instead of unsigned?

In JumpTo and Advance can we not point mCurrent to memory outside of the valid range if we are passed invalid arguments?

Do we want to make JumpTo and Advance return a bool for success/failure and check that everywhere? I'm not sure.

>+  // Determine offset of the 0th IFD. (It shouldn't be greater than 64k, which
>+  // is the maximum size of the entry APP1 segment.)

I think you mean "entire" here.

>+  if (!ReadUInt32(aIFD0OffsetOut) || aIFD0OffsetOut > 64 * 1024)
>+    return false;

Can we clear aIFD0OffsetOut here if it's invalid instead of returning the invalid value?

>+  // The orientation value is 16-bit, right-padded with zeros, but this is a
>+  // 32-bit field, so we need to advance appropriately.
>+  Advance(2);

This comment is a bit confusing. Perhaps "this is a 32 bit field. the orientation is stored as the first 16 bits, the last 16 bits are 0".

Comment 48

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add imgIContainer::GetOrientation.

Moved to part 1 to reflect actual dependencies between part 1 and part 2. Rename Flip::None to Flip::Unflipped to resolve conflict with X.h #define'ing "None".

Comment 49

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add an EXIF parser to imagelib.

Rebased against part 1 and moved to part 2.

Comment 50

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add OrientedImage.

Rebased against the new part 1.

Comment 51

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Timothy!

(In reply to Timothy Nikkel (:tn) from comment #47)
> Can we use an explicit type for length instead of unsigned?

Do you mean uint32_t or the like?

> In JumpTo and Advance can we not point mCurrent to memory outside of the
> valid range if we are passed invalid arguments?

Sure.

> Do we want to make JumpTo and Advance return a bool for success/failure and
> check that everywhere? I'm not sure.

No, I don't think so. Anything that actually reads a value from memory is checking mRemainingLength first; that's where the success/failure comes in. Since we _must_ have those checks regardless (JumpTo and Advance don't know how large the next item to be read is, so they aren't able to complete the job), additional checks in JumpTo and Advance would not add any value.

> >+  if (!ReadUInt32(aIFD0OffsetOut) || aIFD0OffsetOut > 64 * 1024)
> >+    return false;
> 
> Can we clear aIFD0OffsetOut here if it's invalid instead of returning the
> invalid value?

I'd prefer to add an additional variable, but sure.

> >+  // The orientation value is 16-bit, right-padded with zeros, but this is a
> >+  // 32-bit field, so we need to advance appropriately.
> >+  Advance(2);
> 
> This comment is a bit confusing. Perhaps "this is a 32 bit field. the
> orientation is stored as the first 16 bits, the last 16 bits are 0".

Good call.

Comment 52

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add an EXIF parser to imagelib.

Addressed review comments so far.

Comment 53

[Timothy Nikkel (:tnikkel)]

Comment on attachment 793285 [details] [diff] [review]
(Part 2) - Add an EXIF parser to imagelib.

Thanks!

I see where the unsigned type came from now, the jpeg decoder uses it. It would be good if we made the conversion from unsigned (in the jpeg decoder) to uint32_t in the exif decoder explicit (do we have any guarantee on the size of unsigned?).

Comment 54

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #53)
> I see where the unsigned type came from now, the jpeg decoder uses it. It
> would be good if we made the conversion from unsigned (in the jpeg decoder)
> to uint32_t in the exif decoder explicit (do we have any guarantee on the
> size of unsigned?).

Yeah, I agree, I'll add an explicit static_cast. I don't think we can rely on unsigned being 32 bits, but I don't think we currently support or ever will support a platform on which unsigned is _less_ than 32 bits. Given that, static_cast will result in truncation, which should be fine since UINT32_MAX is still __way__ bigger than the maximum segment size the JPEG spec allows. If the value was larger than what would fit in a uint32_t, then even truncated, we would still immediately reject it in the sanity-checking code in EXIFParser::Initialize.

Comment 55

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Parse EXIF orientation in nsJPEGDecoder.

Added an explicit static_cast to address review comments.

Comment 56

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add imgIContainer::GetOrientation.

(Attempt to) fix a compilation issue on Windows by marking GetOrientation as nostdcall in imgIContainer.idl.

Comment 57

[Seth Fowler [:seth] [:s2h]]

OK, everything seems in order here except a compilation issue on Windows which I hope I just solved. Here's an up-to-date try job:

https://tbpl.mozilla.org/?tree=Try&rev=1c76659c795c

Comment 58

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add imgIContainer::GetOrientation.

In the interest of focusing on useful work and not debugging strange Windows-only linker issues, I've just given in and pointlessly implemented GetOrientation in ClippedImage. No idea why this is needed, but it seems to make the Windows build happy.

Comment 59

[Seth Fowler [:seth] [:s2h]]

Final pre-flight check. Aiming to get this in tonight.

https://tbpl.mozilla.org/?tree=Try&rev=f550b601a8f5

Comment 60

[Seth Fowler [:seth] [:s2h]]

Looks good and green to me. Pushed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75abb578641e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4ed3470f04b
https://hg.mozilla.org/integration/mozilla-inbound/rev/76c06d0adfc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/353f5b3c81c7

Comment 61

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc7d8c511a6 since the world changed out from under it.

Comment 62

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add OrientedImage.

Rebased against m-c tip.

Comment 63

[Seth Fowler [:seth] [:s2h]]

Repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/879934b114f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ecfda1b46f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c0d016db6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc41e5dff46

Comment 64

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/879934b114f8
https://hg.mozilla.org/mozilla-central/rev/9ecfda1b46f2
https://hg.mozilla.org/mozilla-central/rev/d6c0d016db6f
https://hg.mozilla.org/mozilla-central/rev/cbc41e5dff46

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 793285 [details] [diff] [review]
(Part 2) - Add an EXIF parser to imagelib.

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

::: image/decoders/EXIF.cpp
@@ +31,5 @@
> +  SignedLongType = 9,
> +  SignedRational = 10,
> +};
> +
> +static const char* EXIFHeader = "Exif\0\0";

static const char EXIFHeader[] = "Exif\0\0";
(saves a relocation)

@@ +78,5 @@
> +EXIFParser::ParseTIFFHeader(uint32_t& aIFD0OffsetOut)
> +{
> +  // Determine byte order.
> +  if (MatchString("MM", 2))
> +    mByteOrder = ByteOrder::BigEndian;

{} around "if" bodies (unless they're control transfer statements, but even then {} works). Similarly below.

@@ +166,5 @@
> +    case 5: aOut = Orientation(Angle::D90,  Flip::Horizontal); break;
> +    case 6: aOut = Orientation(Angle::D90,  Flip::Unflipped);  break;
> +    case 7: aOut = Orientation(Angle::D270, Flip::Horizontal); break;
> +    case 8: aOut = Orientation(Angle::D270, Flip::Unflipped);  break;
> +    default: return false;

Use a static const array of Orientation values instead of code.

@@ +242,5 @@
> +
> +  bool matched;
> +  switch (mByteOrder) {
> +    case ByteOrder::LittleEndian:
> +      matched = mCurrent[0] == low && mCurrent[1] == high;

Seems like it would be simpler to just call ReadUInt16 and then compare the result to aValue.

@@ +263,5 @@
> +EXIFParser::ReadUInt16(uint16_t& aValue)
> +{
> +  if (mRemainingLength < 2)
> +    return false;
> +  

trailing whitespace.

@@ +269,5 @@
> +  switch (mByteOrder) {
> +    case ByteOrder::LittleEndian:
> +      aValue = (uint32_t(mCurrent[1]) << 8) |
> +               (uint32_t(mCurrent[0]));
> +      break;

#include "mozilla/Endian.h" and write LittleEndian::readUint16(mCurrent). Similarly below.

@@ +276,5 @@
> +               (uint32_t(mCurrent[1]));
> +      break;
> +    default:
> +      NS_NOTREACHED("Should know the byte order by now");
> +      matched = false;

'matched' is not needed. Just return false here. Similarly below.