Media Galleries API Metadata: Image metadata

Media Galleries API Metadata: Image metadata

Enables libexif and uses it to parse some image tags into the GetMetadata call.

BUG=318450
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260917

[tommycli, 2014-03-11 22:41:30]

+vandebo cc if he's curious

[Lei Zhang, 2014-03-11 23:38:42]

https://codereview.chromium.org/191583002/diff/40001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/191583002/diff/40001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:301: 'utility/media_galleries/image_metadata_extractor.cc',
nit: alphabetical order

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
File chrome/utility/media_galleries/image_metadata_extractor.cc (right):

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor.cc:30: if
(media::DataSource::kReadError == bytes_read) {
bytes_read == media::DataSource::kReadError

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor.cc:71: switch
(entry->format) {
Just DCHECK_EQ(EXIF_FORMAT_SHORT, entry->format) and drop the switch() ?

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
File chrome/utility/media_galleries/image_metadata_extractor_unittest.cc
(right):

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor_unittest.cc:24: path =
path.AppendASCII("chrome").AppendASCII("test").AppendASCII("data")
you can omit some of this and use chrome::DIR_TEST_DATA.

[tommycli, 2014-03-12 00:09:04]

https://codereview.chromium.org/191583002/diff/40001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/191583002/diff/40001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:301: 'utility/media_galleries/image_metadata_extractor.cc',
On 2014/03/11 23:38:43, Lei Zhang wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
File chrome/utility/media_galleries/image_metadata_extractor.cc (right):

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor.cc:30: if
(media::DataSource::kReadError == bytes_read) {
On 2014/03/11 23:38:43, Lei Zhang wrote:
> bytes_read == media::DataSource::kReadError

Done.

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor.cc:71: switch
(entry->format) {
On 2014/03/11 23:38:43, Lei Zhang wrote:
> Just DCHECK_EQ(EXIF_FORMAT_SHORT, entry->format) and drop the switch() ?

I don't want to actually crash if the ExifEntry is not to spec. I'm happy to
just leave it set at -1. What do you think?

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
File chrome/utility/media_galleries/image_metadata_extractor_unittest.cc
(right):

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor_unittest.cc:24: path =
path.AppendASCII("chrome").AppendASCII("test").AppendASCII("data")
On 2014/03/11 23:38:43, Lei Zhang wrote:
> you can omit some of this and use chrome::DIR_TEST_DATA.

Done.

[Lei Zhang, 2014-03-12 01:55:29]

My other concern is - should we bundle our own libexif on Linux, or require the
system to provide it?

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
File chrome/utility/media_galleries/image_metadata_extractor.cc (right):

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor.cc:71: switch
(entry->format) {
On 2014/03/12 00:09:05, tommycli wrote:
> On 2014/03/11 23:38:43, Lei Zhang wrote:
> > Just DCHECK_EQ(EXIF_FORMAT_SHORT, entry->format) and drop the switch() ?
> 
> I don't want to actually crash if the ExifEntry is not to spec. I'm happy to
> just leave it set at -1. What do you think?

Is your concern that the NOTREACHED() case will actually get reached in the real
world with real data? If so, it's not actually NOTREACHED(), because it's
reachable.

In any case, I'd change from switch() to an if() since there is only 1 value of
entry->format that is relevant.

[tommycli, 2014-03-12 16:27:48]

thestig: Hi, I added a tentative patchset 6 to use the system libexif. Let me
know what yo uthink.

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
File chrome/utility/media_galleries/image_metadata_extractor.cc (right):

https://codereview.chromium.org/191583002/diff/40001/chrome/utility/media_gal...
chrome/utility/media_galleries/image_metadata_extractor.cc:71: switch
(entry->format) {
On 2014/03/12 01:55:29, Lei Zhang wrote:
> On 2014/03/12 00:09:05, tommycli wrote:
> > On 2014/03/11 23:38:43, Lei Zhang wrote:
> > > Just DCHECK_EQ(EXIF_FORMAT_SHORT, entry->format) and drop the switch() ?
> > 
> > I don't want to actually crash if the ExifEntry is not to spec. I'm happy to
> > just leave it set at -1. What do you think?
> 
> Is your concern that the NOTREACHED() case will actually get reached in the
real
> world with real data? If so, it's not actually NOTREACHED(), because it's
> reachable.
> 
> In any case, I'd change from switch() to an if() since there is only 1 value
of
> entry->format that is relevant.

Oh you're right. I made a mistake. I realized I also need to handle
EXIF_FORMAT_LONG values, since images seem to have varying entry formats...

[Lei Zhang, 2014-03-12 22:49:45]

https://codereview.chromium.org/191583002/diff/90001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/191583002/diff/90001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:359: '../third_party/libexif/libexif.gyp:libexif',
Do we try to build utility/media_galleries/* on Android?

https://codereview.chromium.org/191583002/diff/90001/third_party/libexif/libe...
File third_party/libexif/libexif.gyp (right):

https://codereview.chromium.org/191583002/diff/90001/third_party/libexif/libe...
third_party/libexif/libexif.gyp:8: ['os_posix == 1 and OS != "mac" and OS !=
"ios" and OS != "android"', {
Maybe just do: OS == Linux ?

https://codereview.chromium.org/191583002/diff/100001/build/install-build-dep...
File build/install-build-deps.sh (right):

https://codereview.chromium.org/191583002/diff/100001/build/install-build-dep...
build/install-build-deps.sh:101: libdrm-dev libelf-dev libexif-dev libgconf2-dev
libgl1-mesa-dev
You want changes to this file in a separate CL. So you can land it separately
and have the buildbots updated before landing the rest of the CL.

[tommycli, 2014-03-12 23:27:20]

https://codereview.chromium.org/191583002/diff/90001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/191583002/diff/90001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:359: '../third_party/libexif/libexif.gyp:libexif',
On 2014/03/12 22:49:46, Lei Zhang wrote:
> Do we try to build utility/media_galleries/* on Android?

Don't think so. See the the source exclude a few lines down.

https://codereview.chromium.org/191583002/diff/90001/third_party/libexif/libe...
File third_party/libexif/libexif.gyp (right):

https://codereview.chromium.org/191583002/diff/90001/third_party/libexif/libe...
third_party/libexif/libexif.gyp:8: ['os_posix == 1 and OS != "mac" and OS !=
"ios" and OS != "android"', {
On 2014/03/12 22:49:46, Lei Zhang wrote:
> Maybe just do: OS == Linux ?

Works with me. What's the situation with ChromeOS if we do this?

https://codereview.chromium.org/191583002/diff/100001/build/install-build-dep...
File build/install-build-deps.sh (right):

https://codereview.chromium.org/191583002/diff/100001/build/install-build-dep...
build/install-build-deps.sh:101: libdrm-dev libelf-dev libexif-dev libgconf2-dev
libgl1-mesa-dev
On 2014/03/12 22:49:46, Lei Zhang wrote:
> You want changes to this file in a separate CL. So you can land it separately
> and have the buildbots updated before landing the rest of the CL.

Done.

[tommycli, 2014-03-13 21:44:43]

On 2014/03/12 23:27:20, tommycli wrote:
> https://codereview.chromium.org/191583002/diff/90001/chrome/chrome.gyp
> File chrome/chrome.gyp (right):
> 
>
https://codereview.chromium.org/191583002/diff/90001/chrome/chrome.gyp#newcod...
> chrome/chrome.gyp:359: '../third_party/libexif/libexif.gyp:libexif',
> On 2014/03/12 22:49:46, Lei Zhang wrote:
> > Do we try to build utility/media_galleries/* on Android?
> 
> Don't think so. See the the source exclude a few lines down.
> 
>
https://codereview.chromium.org/191583002/diff/90001/third_party/libexif/libe...
> File third_party/libexif/libexif.gyp (right):
> 
>
https://codereview.chromium.org/191583002/diff/90001/third_party/libexif/libe...
> third_party/libexif/libexif.gyp:8: ['os_posix == 1 and OS != "mac" and OS !=
> "ios" and OS != "android"', {
> On 2014/03/12 22:49:46, Lei Zhang wrote:
> > Maybe just do: OS == Linux ?
> 
> Works with me. What's the situation with ChromeOS if we do this?
> 
>
https://codereview.chromium.org/191583002/diff/100001/build/install-build-dep...
> File build/install-build-deps.sh (right):
> 
>
https://codereview.chromium.org/191583002/diff/100001/build/install-build-dep...
> build/install-build-deps.sh:101: libdrm-dev libelf-dev libexif-dev
libgconf2-dev
> libgl1-mesa-dev
> On 2014/03/12 22:49:46, Lei Zhang wrote:
> > You want changes to this file in a separate CL. So you can land it
separately
> > and have the buildbots updated before landing the rest of the CL.
> 
> Done.

vapier@ on ChromeOS use_system_libexif:
if we had anything else in the system using it, it'd make sense to share.  but
if the only thing is chromium, then statically linking it would probably be
better

[tommycli, 2014-03-24 18:14:17]

thestig: PTAL. After much gnashing of teeth it looks like this patch is on the
way to success...

libexif-dev package has been added to the linux buildbots already.

[Lei Zhang, 2014-03-24 19:29:06]

You are missing changes to chrome/installer/linux for sure. I'm not 100% certain
on the Win/Mac changes, but they look ok to me.

[tommycli, 2014-03-24 21:46:43]

I added the installer/linux changes.

[Lei Zhang, 2014-03-24 21:52:48]

On 2014/03/24 21:46:43, tommycli wrote:
> I added the installer/linux changes.

Thanks, but you probably want the chrome/installer/linux/sysroot_scripts changes
in a separate CL. I can generate new sysroots for you.

[Lei Zhang, 2014-03-26 17:32:28]

lgtm

[tommycli, 2014-03-26 17:36:44]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:37:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/191583002/360001

[commit-bot: I haz the power, 2014-03-26 19:06:24]

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

[commit-bot: I haz the power, 2014-03-26 19:06:25]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[tommycli, 2014-03-26 21:05:51]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-03-26 21:07:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/191583002/360001

[tommycli, 2014-03-26 23:19:25]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-03-26 23:20:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/191583002/400001

[commit-bot: I haz the power, 2014-03-27 05:16:48]

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

[commit-bot: I haz the power, 2014-03-27 05:16:49]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[tommycli, 2014-03-27 23:26:10]

Hi groby,

I'm hitting a wall getting this libexif working on Mac. Been trying for a few
days straight now. I listed my "question-marks" below. I figured I should bring
in some artillery now..

Thanks for your help

https://codereview.chromium.org/191583002/diff/460001/chrome/chrome.isolate
File chrome/chrome.isolate (right):

https://codereview.chromium.org/191583002/diff/460001/chrome/chrome.isolate#n...
chrome/chrome.isolate:50: '<(PRODUCT_DIR)/libexif.dylib',
Not sure if this is needed, since this is copied to Chromium.app/

https://codereview.chromium.org/191583002/diff/460001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/191583002/diff/460001/chrome/chrome_exe.gypi#...
chrome/chrome_exe.gypi:357: '<(PRODUCT_DIR)/libexif.dylib',
This copies the dylib into the same place as libplugin_carbon_interpose.dylib,
but I'm not sure if this is correct either.

https://codereview.chromium.org/191583002/diff/460001/third_party/libexif/lib...
File third_party/libexif/libexif.gyp (right):

https://codereview.chromium.org/191583002/diff/460001/third_party/libexif/lib...
third_party/libexif/libexif.gyp:86: 'DYLIB_INSTALL_NAME_BASE': '@loader_path',
I changed this from @executable_path/../../.. because out/Debug/unit_tests, etc.
would not work until I changed it to @loader_path, and I saw @loader_path used a
few other places in Chrome. Not sure what I'm doing here.

https://codereview.chromium.org/191583002/diff/460001/third_party/libexif/lib...
third_party/libexif/libexif.gyp:88: 'postbuilds': [
I also copied this from chrome.gyp:678 because it seemed like
libplugin_carbon_interpose did this. Again, not sure of the purpose.

[tommycli, 2014-03-27 23:28:58]

Also the tests that are failing work locally, so it's possibly an issue with my
isolate files.

On 2014/03/27 23:26:10, tommycli wrote:
> Hi groby,
> 
> I'm hitting a wall getting this libexif working on Mac. Been trying for a few
> days straight now. I listed my "question-marks" below. I figured I should
bring
> in some artillery now..
> 
> Thanks for your help
> 
> https://codereview.chromium.org/191583002/diff/460001/chrome/chrome.isolate
> File chrome/chrome.isolate (right):
> 
>
https://codereview.chromium.org/191583002/diff/460001/chrome/chrome.isolate#n...
> chrome/chrome.isolate:50: '<(PRODUCT_DIR)/libexif.dylib',
> Not sure if this is needed, since this is copied to Chromium.app/
> 
> https://codereview.chromium.org/191583002/diff/460001/chrome/chrome_exe.gypi
> File chrome/chrome_exe.gypi (right):
> 
>
https://codereview.chromium.org/191583002/diff/460001/chrome/chrome_exe.gypi#...
> chrome/chrome_exe.gypi:357: '<(PRODUCT_DIR)/libexif.dylib',
> This copies the dylib into the same place as libplugin_carbon_interpose.dylib,
> but I'm not sure if this is correct either.
> 
>
https://codereview.chromium.org/191583002/diff/460001/third_party/libexif/lib...
> File third_party/libexif/libexif.gyp (right):
> 
>
https://codereview.chromium.org/191583002/diff/460001/third_party/libexif/lib...
> third_party/libexif/libexif.gyp:86: 'DYLIB_INSTALL_NAME_BASE': '@loader_path',
> I changed this from @executable_path/../../.. because out/Debug/unit_tests,
etc.
> would not work until I changed it to @loader_path, and I saw @loader_path used
a
> few other places in Chrome. Not sure what I'm doing here.
> 
>
https://codereview.chromium.org/191583002/diff/460001/third_party/libexif/lib...
> third_party/libexif/libexif.gyp:88: 'postbuilds': [
> I also copied this from chrome.gyp:678 because it seemed like
> libplugin_carbon_interpose did this. Again, not sure of the purpose.

[groby-ooo-7-16, 2014-03-28 00:09:45]

I'm not exactly at home with this, but here's what I do know


* You want your library as part of the framework. That's done in
chrome_dll_bundle.gypi - pattern after ffmpegsumo.so. If it's not in there,
it'll fail to pick it up when loading. Which is what breaks your tests.

* You might want this in content_shell, too. (I think content_shell _should_
pull in media gallery functionality, right?)

* Since "Chromium Frameworks.framework" is an untracked dependency for all
isolates anyways, you migth not need to add the .so file in <(PRODUCT_DIR) - I
think.  But I'm not sure about that - maruel might know better. (Or just leave
it in. We do have an ffmpegsumo.so dependency there, too)

[tommycli, 2014-03-29 17:54:28]

groby: Following your advice on the Framework/dylib issues, I think I finally
have it working right on Mac.

PTAL. I'll send to CQ end of monday unless you have objections.

[tommycli, 2014-04-01 00:14:28]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/191583002/540001

[commit-bot: I haz the power, 2014-04-01 00:15:44]

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

[commit-bot: I haz the power, 2014-04-01 00:15:45]

Failed to apply patch for chrome/chrome.isolate:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome.isolate
  Hunk #1 FAILED at 66.
  1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome.isolate.rej

Patch:       chrome/chrome.isolate
Index: chrome/chrome.isolate
diff --git a/chrome/chrome.isolate b/chrome/chrome.isolate
index
104f47123e7d068870a992e87b3e02e4ddc0b45c..80840068d200a1db9888cdd076f5158806ea6246
100644
--- a/chrome/chrome.isolate
+++ b/chrome/chrome.isolate
@@ -66,6 +66,7 @@
           '<(PRODUCT_DIR)/ffmpegsumo.dll',
           '<(PRODUCT_DIR)/libEGL.dll',
           '<(PRODUCT_DIR)/libGLESv2.dll',
+          '<(PRODUCT_DIR)/libexif.dll',
           '<(PRODUCT_DIR)/mojo_system.dll',
           '<(PRODUCT_DIR)/nacl64<(EXECUTABLE_SUFFIX)',
           '<(PRODUCT_DIR)/osmesa.dll',

[groby-ooo-7-16, 2014-04-01 00:53:12]

With the limited gypi/isolate knowledge I have, this seems to be OK now.

I guess that means LGTM ;)

[tommycli, 2014-04-01 16:18:24]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-04-01 16:18:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/191583002/570001

[commit-bot: I haz the power, 2014-04-01 19:13:44]

Change committed as 260917

[boliu, 2014-04-02 01:15:53]

I believe after this change, gyp on linux requires libexif-dev package on ubuntu
12.04. Should add it to build/install-build-deps.sh and worth a notice to
chromium-dev?

I got an error like this without the package:

Updating projects from gyp files...
Package libexif was not found in the pkg-config search path.
Perhaps you should add the directory containing `libexif.pc'
to the PKG_CONFIG_PATH environment variable
No package 'libexif' found
gyp: Call to 'pkg-config --cflags libexif' returned exit status 1.

[Lei Zhang, 2014-04-02 07:07:21]

On 2014/04/02 01:15:53, boliu wrote:
> I believe after this change, gyp on linux requires libexif-dev package on
ubuntu
> 12.04. Should add it to build/install-build-deps.sh and worth a notice to
> chromium-dev?
> 
> I got an error like this without the package:
> 
> Updating projects from gyp files...
> Package libexif was not found in the pkg-config search path.
> Perhaps you should add the directory containing `libexif.pc'
> to the PKG_CONFIG_PATH environment variable
> No package 'libexif' found
> gyp: Call to 'pkg-config --cflags libexif' returned exit status 1.

It's in install-build-deps.sh but we forgot to send an email. Will do now.