Parse icc profiles and exif orientation from jpeg markers

Parse icc profiles and exif orientation from jpeg markers

New resources should be fine to add since they are already
checked into chromium.

BUG=skia:3834
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1813273002

Committed: https://skia.googlesource.com/skia/+/0e6274f54084d816abd70d445dded8657eb01abd

[msarett, 2016-03-18 16:50:30]

Description was changed from

==========
Parse icc profiles and exif orientation from jpeg markers

BUG=skia:
==========

to

==========
Parse icc profiles and exif orientation from jpeg markers

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-03-18 18:57:04]

Patchset #1 (id:1) has been deleted

[msarett, 2016-03-18 18:58:53]

Description was changed from

==========
Parse icc profiles and exif orientation from jpeg markers

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Parse icc profiles and exif orientation from jpeg markers

New resources should be fine to add since they are already
checked into chromium.

BUG=skia:3834
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-03-18 19:16:57]

Patchset #1 (id:20001) has been deleted

[msarett, 2016-03-18 19:34:29]

Patchset #1 (id:40001) has been deleted

[msarett, 2016-03-18 19:37:02]

msarett@google.com changed reviewers:
+ reed@google.com, scroggo@google.com

[msarett, 2016-03-18 19:37:02]

https://codereview.chromium.org/1813273002/diff/60001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1813273002/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:235: SkImageOrientation orientation =
get_exif_orientation(decoderMgr->dinfo());
I had originally planned to combine these calls into a single pass over the
markers.

But I think the code already looks complex and I didn't want to make it even
more so.

[reed1, 2016-03-18 19:44:22]

If you're going to scope the enum name with SkImage..., you should make it
global. If you want the enum to be just part of SkCodec, you can simplify the
name...

SkCodec {

    enum Orientation {
        kLeftBottom_Orientation,
       ...
    };

};

[msarett, 2016-03-18 21:15:28]

On 2016/03/18 19:44:22, reed1 wrote:
> If you're going to scope the enum name with SkImage..., you should make it
> global. If you want the enum to be just part of SkCodec, you can simplify the
> name...
> 
> SkCodec {
> 
>     enum Orientation {
>         kLeftBottom_Orientation,
>        ...
>     };
> 
> };

Gotcha.  For now, I'm making it SkCodec::Orientation.  I can rename later if we
decide to move it out of SkCodec.

[scroggo, 2016-03-18 22:38:18]

lgtm

https://codereview.chromium.org/1813273002/diff/80001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1813273002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:513: *  Does not affect ownership of SkColorSpace*
This comment can be removed, I think.

https://codereview.chromium.org/1813273002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:650: Orientation                 fOrientation;
Can this be const?

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:64: // II indicates Intel (little endian) and MM
indicates motorola (big endian).
This could potentially be a common method. This also shows up in SkRawCodec.

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:68: bool littleEndian = ('I' == data[6]);
nit: could be const.

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:123: if (memcmp(marker->data, kICCSig,
sizeof(kICCSig))) {
Why not

return !memcmp(...)

https://codereview.chromium.org/1813273002/diff/80001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/1813273002/diff/80001/tests/ColorSpaceTest.cp...
tests/ColorSpaceTest.cpp:64: SkColorSpace* colorSpace = codec->getColorSpace();
Should this method return a

const sk_sp<SkColorSpace>&?

[msarett, 2016-03-21 14:28:46]

https://codereview.chromium.org/1813273002/diff/80001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1813273002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:513: *  Does not affect ownership of SkColorSpace*
On 2016/03/18 22:38:17, scroggo wrote:
> This comment can be removed, I think.

Done.

https://codereview.chromium.org/1813273002/diff/80001/include/codec/SkCodec.h...
include/codec/SkCodec.h:650: Orientation                 fOrientation;
On 2016/03/18 22:38:17, scroggo wrote:
> Can this be const?

Yes!  Done.

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:64: // II indicates Intel (little endian) and MM
indicates motorola (big endian).
On 2016/03/18 22:38:17, scroggo wrote:
> This could potentially be a common method. This also shows up in SkRawCodec.

Done.

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:68: bool littleEndian = ('I' == data[6]);
On 2016/03/18 22:38:17, scroggo wrote:
> nit: could be const.

Acknowledged.  I've changed the implementation.

https://codereview.chromium.org/1813273002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:123: if (memcmp(marker->data, kICCSig,
sizeof(kICCSig))) {
On 2016/03/18 22:38:17, scroggo wrote:
> Why not
> 
> return !memcmp(...)

Agreed. Fixing this.

https://codereview.chromium.org/1813273002/diff/80001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/1813273002/diff/80001/tests/ColorSpaceTest.cp...
tests/ColorSpaceTest.cpp:64: SkColorSpace* colorSpace = codec->getColorSpace();
On 2016/03/18 22:38:17, scroggo wrote:
> Should this method return a
> 
> const sk_sp<SkColorSpace>&?

As we discussed in person, I think this is the right change, but I'll leave it
for a separate CL.

[scroggo, 2016-03-21 14:31:46]

lgtm patch set 3

[msarett, 2016-03-21 14:35:30]

Mike, can you take a look at the API?

[reed1, 2016-03-21 14:37:28]

lgtm

stupid bikeshed:

every enum has "Origin" and "Orientation"

what about

enum Origin {
    kTopLeft_Origin,
    ....
};

[msarett, 2016-03-21 14:41:52]

On 2016/03/21 14:37:28, reed1 wrote:
> lgtm
> 
> stupid bikeshed:
> 
> every enum has "Origin" and "Orientation"
> 
> what about
> 
> enum Origin {
>     kTopLeft_Origin,
>     ....
> };

sgtm

Would you prefer the getter/variable names also be updated to get getOrigin()
etc.?

[msarett, 2016-03-21 14:47:54]

Changed all references to "Orientation" to "Origin"

[msarett, 2016-03-21 14:48:12]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 14:48:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813273002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813273002/120001

[commit-bot: I haz the power, 2016-03-21 14:49:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 14:49:49]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[msarett, 2016-03-21 14:51:35]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 14:51:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813273002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813273002/140001

[reed1, 2016-03-21 15:01:38]

lgtm

[commit-bot: I haz the power, 2016-03-21 15:03:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 15:03:06]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-03-21 15:03:33]

The CQ bit was checked by msarett@google.com

[msarett, 2016-03-21 15:03:34]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1813273002/#ps140001
(title: "Fix build error")

[commit-bot: I haz the power, 2016-03-21 15:03:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813273002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813273002/140001

[commit-bot: I haz the power, 2016-03-21 15:04:42]

Description was changed from

==========
Parse icc profiles and exif orientation from jpeg markers

New resources should be fine to add since they are already
checked into chromium.

BUG=skia:3834
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Parse icc profiles and exif orientation from jpeg markers

New resources should be fine to add since they are already
checked into chromium.

BUG=skia:3834
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/0e6274f54084d816abd70d445dded8657eb01abd
==========

[commit-bot: I haz the power, 2016-03-21 15:04:43]

Committed patchset #5 (id:140001) as
https://skia.googlesource.com/skia/+/0e6274f54084d816abd70d445dded8657eb01abd