Compile Skia image encoders

Compile Skia image encoders

BUG=713862
Review-Url: https://codereview.chromium.org/2865363002
Cr-Commit-Position: refs/heads/master@{#471382}
Committed: https://chromium.googlesource.com/chromium/src/+/b06d81fdf9fc3a85fe95aa3ef9cd1d7de09758c8

[msarett1, 2017-05-10 19:52:20]

Patchset #3 (id:40001) has been deleted

[msarett1, 2017-05-10 19:52:27]

Patchset #2 (id:20001) has been deleted

[msarett1, 2017-05-10 19:52:34]

Patchset #1 (id:1) has been deleted

[msarett1, 2017-05-10 19:56:37]

msarett@chromium.org changed reviewers:
+ halcanary@google.com, msarett@chromium.org, reed@google.com

[msarett1, 2017-05-10 19:56:38]

This just compiles Skia's encoders in Chrome and uses them for XPS.  No need to
look at the webp changes - I'll add a reviewer for those if this looks ok.

[hal.canary, 2017-05-10 20:15:09]

On 2017/05/10 19:56:38, msarett1 wrote:
> This just compiles Skia's encoders in Chrome and uses them for XPS.  No need
to
> look at the webp changes - I'll add a reviewer for those if this looks ok.

right now, I only need PNG.

[hal.canary, 2017-05-10 20:17:27]

On 2017/05/10 20:15:09, hal.canary wrote:
> On 2017/05/10 19:56:38, msarett1 wrote:
> > This just compiles Skia's encoders in Chrome and uses them for XPS.  No need
> to
> > look at the webp changes - I'll add a reviewer for those if this looks ok.
> 
> right now, I only need PNG.

actually, src/xps/SkXPSDevice.cpp:631 could call SkPngEncoder::Encode().  Do you
have a preference?

[msarett1, 2017-05-10 20:24:16]

"right now, I only need PNG."

Got it.  I'll eventually need them all, so that's why I'm adding all three.

"actually, src/xps/SkXPSDevice.cpp:631 could call SkPngEncoder::Encode(). Do you
have a preference?"

SkPngEncoder::Encode() actually is my preference.  Though, IIUC, we'll need to
land this first before making that change.

[reed1, 2017-05-11 13:18:21]

lgtm

[msarett1, 2017-05-11 13:27:08]

msarett@chromium.org changed reviewers:
+ bungeman@google.com, mtklein@google.com

[msarett1, 2017-05-11 13:27:09]

Ben and Mike, would you mind taking a look at skia/BUILD.gn?  It would be good
to get a general review, and I also have an include dirs question with png.h.

https://codereview.chromium.org/2865363002/diff/60001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/2865363002/diff/60001/skia/BUILD.gn#newcode414
skia/BUILD.gn:414: "//third_party/libpng:libpng_config",
I'm not sure why this is necessary.  I needed it to get the compile to succeed
locally.

Without this change, some system version of png.h is included instead of the
third_party/libpng/png.h.  Any idea what might be going here?  It looks to me
like we should get the proper include directories just by depending on libpng.

[mtklein_C, 2017-05-11 13:52:45]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2017-05-11 13:52:46]

https://codereview.chromium.org/2865363002/diff/60001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/2865363002/diff/60001/skia/BUILD.gn#newcode414
skia/BUILD.gn:414: "//third_party/libpng:libpng_config",
On 2017/05/11 at 13:27:08, msarett1 wrote:
> I'm not sure why this is necessary.  I needed it to get the compile to succeed
locally.
> 
> Without this change, some system version of png.h is included instead of the
third_party/libpng/png.h.  Any idea what might be going here?  It looks to me
like we should get the proper include directories just by depending on libpng.

Does this seem like the right explanation?

   - :libpng_config provides the include path
   - :libpng_sources public_configs on :libpng_config, adding :libpng_config its
own configs and to the configs of anyone who directly depends on :libpng_sources
   - :libpng public_deps on :libpng_sources, adding a dependency on
:libpng_sources if you depend on :libpng, but not a direct one.

If you're playing around, you might try adding a public_configs on
:libpng_config to :libpng, and see if that lets you remove your own manual one
in :skia.  If it were me I'd probably find whoever wrote the BUILD.gn file to
see if this was intentional that you need to depend on both :libpng and
:libpng_config to use the library... it's possible that this is some sort of
convention Chromium uses?

[msarett1, 2017-05-11 15:34:15]

On 2017/05/11 13:52:46, mtklein_C wrote:
> https://codereview.chromium.org/2865363002/diff/60001/skia/BUILD.gn
> File skia/BUILD.gn (right):
> 
> https://codereview.chromium.org/2865363002/diff/60001/skia/BUILD.gn#newcode414
> skia/BUILD.gn:414: "//third_party/libpng:libpng_config",
> On 2017/05/11 at 13:27:08, msarett1 wrote:
> > I'm not sure why this is necessary.  I needed it to get the compile to
succeed
> locally.
> > 
> > Without this change, some system version of png.h is included instead of the
> third_party/libpng/png.h.  Any idea what might be going here?  It looks to me
> like we should get the proper include directories just by depending on libpng.
> 
> Does this seem like the right explanation?
> 
>    - :libpng_config provides the include path
>    - :libpng_sources public_configs on :libpng_config, adding :libpng_config
its
> own configs and to the configs of anyone who directly depends on
:libpng_sources
>    - :libpng public_deps on :libpng_sources, adding a dependency on
> :libpng_sources if you depend on :libpng, but not a direct one.
> 
> If you're playing around, you might try adding a public_configs on
> :libpng_config to :libpng, and see if that lets you remove your own manual one
> in :skia.  If it were me I'd probably find whoever wrote the BUILD.gn file to
> see if this was intentional that you need to depend on both :libpng and
> :libpng_config to use the library... it's possible that this is some sort of
> convention Chromium uses?

Thanks, that seems to make sense.

I modified the libpng group to:
  group("libpng") {
    public_deps = [
      ":libpng_sources",
    ]
    public_configs = [
      ":libpng_config",
    ]
  }

Unfortunately, that doesn't allow me to remove the manual include in Skia. 
Strangely, other clients of libpng don't seem to need to manually include the
config.

[mtklein_C, 2017-05-11 15:37:01]

> Thanks, that seems to make sense.
> 
> I modified the libpng group to:
>   group("libpng") {
>     public_deps = [
>       ":libpng_sources",
>     ]
>     public_configs = [
>       ":libpng_config",
>     ]
>   }
> 
> Unfortunately, that doesn't allow me to remove the manual include in Skia. 
Strangely, other clients of libpng don't seem to need to manually include the
config.

Well darn.  That does not match my model of how things should work.

Is it possible that the other clients don't care which png.h they're getting,
and they just get the wrong one too?

[msarett1, 2017-05-11 16:11:39]

On 2017/05/11 15:37:01, mtklein_C wrote:
> > Thanks, that seems to make sense.
> > 
> > I modified the libpng group to:
> >   group("libpng") {
> >     public_deps = [
> >       ":libpng_sources",
> >     ]
> >     public_configs = [
> >       ":libpng_config",
> >     ]
> >   }
> > 
> > Unfortunately, that doesn't allow me to remove the manual include in Skia. 
> Strangely, other clients of libpng don't seem to need to manually include the
> config.
> 
> Well darn.  That does not match my model of how things should work.
> 
> Is it possible that the other clients don't care which png.h they're getting,
> and they just get the wrong one too?

I think maybe depending on the order in which things are compiled.

I'm having issues because we're including the wrong png.h before we've seen
pngprefix.h (all of the functions should have a cr_ prefix).  If other targets
are compiled after libpng's pngprefix.h is processed, maybe that would allow
them to use the wrong header file but still get png prefixing?

[msarett1, 2017-05-11 19:05:34]

On 2017/05/11 16:11:39, msarett1 wrote:
> On 2017/05/11 15:37:01, mtklein_C wrote:
> > > Thanks, that seems to make sense.
> > > 
> > > I modified the libpng group to:
> > >   group("libpng") {
> > >     public_deps = [
> > >       ":libpng_sources",
> > >     ]
> > >     public_configs = [
> > >       ":libpng_config",
> > >     ]
> > >   }
> > > 
> > > Unfortunately, that doesn't allow me to remove the manual include in Skia.

> > Strangely, other clients of libpng don't seem to need to manually include
the
> > config.
> > 
> > Well darn.  That does not match my model of how things should work.
> > 
> > Is it possible that the other clients don't care which png.h they're
getting,
> > and they just get the wrong one too?
> 
> I think maybe depending on the order in which things are compiled.
> 
> I'm having issues because we're including the wrong png.h before we've seen
> pngprefix.h (all of the functions should have a cr_ prefix).  If other targets
> are compiled after libpng's pngprefix.h is processed, maybe that would allow
> them to use the wrong header file but still get png prefixing?

Woo, ok I figured it out.

"//build/config/linux/pangocairo" has its own png.h.  We need to make sure the
path to the real png.h is included before that.

[reed1, 2017-05-11 19:06:59]

On 2017/05/11 19:05:34, msarett1 wrote:
> On 2017/05/11 16:11:39, msarett1 wrote:
> > On 2017/05/11 15:37:01, mtklein_C wrote:
> > > > Thanks, that seems to make sense.
> > > > 
> > > > I modified the libpng group to:
> > > >   group("libpng") {
> > > >     public_deps = [
> > > >       ":libpng_sources",
> > > >     ]
> > > >     public_configs = [
> > > >       ":libpng_config",
> > > >     ]
> > > >   }
> > > > 
> > > > Unfortunately, that doesn't allow me to remove the manual include in
Skia.
> 
> > > Strangely, other clients of libpng don't seem to need to manually include
> the
> > > config.
> > > 
> > > Well darn.  That does not match my model of how things should work.
> > > 
> > > Is it possible that the other clients don't care which png.h they're
> getting,
> > > and they just get the wrong one too?
> > 
> > I think maybe depending on the order in which things are compiled.
> > 
> > I'm having issues because we're including the wrong png.h before we've seen
> > pngprefix.h (all of the functions should have a cr_ prefix).  If other
targets
> > are compiled after libpng's pngprefix.h is processed, maybe that would allow
> > them to use the wrong header file but still get png prefixing?
> 
> Woo, ok I figured it out.
> 
> "//build/config/linux/pangocairo" has its own png.h.  We need to make sure the
> path to the real png.h is included before that.

wow, good work.

can we obsolete our need for pango? (future CL :)

[msarett1, 2017-05-11 19:10:47]

msarett@chromium.org changed reviewers:
+ erg@chromium.org, jbauman@chromium.org, urvang@chromium.org

[msarett1, 2017-05-11 19:10:48]

Adding OWNERS, please take a look.

urvang@: third_party/libwebp
jbauman@: content/renderer/gpu
erg@: ui/gfx/codec

[urvang, 2017-05-11 19:23:07]

LGTM for WebP

[Elliot Glaysher, 2017-05-11 19:40:43]

lgtm

Is the long term plan to deprecate ui/gfx/codec entirely?

https://codereview.chromium.org/2865363002/diff/80001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/2865363002/diff/80001/skia/BUILD.gn#newcode410
skia/BUILD.gn:410: #configs -= [ "//build/config/compiler:chromium_code" ]
just delete this; don't leave commented out code

https://codereview.chromium.org/2865363002/diff/80001/skia/BUILD.gn#newcode432
skia/BUILD.gn:432: # ensure that it is included before the pangocairo path
(which also has a png.h).
80 cols

[msarett1, 2017-05-11 20:01:27]

"Is the long term plan to deprecate ui/gfx/codec entirely?"

Yes!  Though I'm actually starting with platform/image-encoder.

https://codereview.chromium.org/2865363002/diff/80001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/2865363002/diff/80001/skia/BUILD.gn#newcode410
skia/BUILD.gn:410: #configs -= [ "//build/config/compiler:chromium_code" ]
On 2017/05/11 19:40:43, Elliot Glaysher wrote:
> just delete this; don't leave commented out code

Oops, that is left over from my experiments.  Putting this back in, thanks for
catching.

https://codereview.chromium.org/2865363002/diff/80001/skia/BUILD.gn#newcode432
skia/BUILD.gn:432: # ensure that it is included before the pangocairo path
(which also has a png.h).
On 2017/05/11 19:40:43, Elliot Glaysher wrote:
> 80 cols

Done.

[msarett1, 2017-05-11 20:40:41]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 20:41:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 20:53:41]

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

[commit-bot: I haz the power, 2017-05-11 20:53:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jbauman, 2017-05-11 22:03:00]

content/renderer/gpu lgtm

[msarett1, 2017-05-12 12:57:31]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 12:57:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 13:05:27]

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

[commit-bot: I haz the power, 2017-05-12 13:05:28]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[msarett1, 2017-05-12 13:06:35]

Patchset #4 (id:120001) has been deleted

[msarett1, 2017-05-12 13:15:06]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 13:15:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 14:28:38]

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

[commit-bot: I haz the power, 2017-05-12 14:28:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[msarett1, 2017-05-12 17:02:35]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 17:04:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 18:20:02]

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

[commit-bot: I haz the power, 2017-05-12 18:20:03]

Dry run: This issue passed the CQ dry run.

[msarett1, 2017-05-12 18:20:43]

The CQ bit was checked by msarett@chromium.org

[msarett1, 2017-05-12 18:20:43]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
urvang@chromium.org, erg@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2865363002/#ps140001
(title: "Fix ios and win")

[commit-bot: I haz the power, 2017-05-12 18:21:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 18:42:51]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494613243300010,
"parent_rev": "460d2176609a29589b5428b447a8f6b7d0a1fef4", "commit_rev":
"b06d81fdf9fc3a85fe95aa3ef9cd1d7de09758c8"}

[commit-bot: I haz the power, 2017-05-12 18:43:00]

Description was changed from

==========
Compile Skia image encoders

BUG=713862
==========

to

==========
Compile Skia image encoders

BUG=713862

Review-Url: https://codereview.chromium.org/2865363002
Cr-Commit-Position: refs/heads/master@{#471382}
Committed:
https://chromium.googlesource.com/chromium/src/+/b06d81fdf9fc3a85fe95aa3ef9cd...
==========

[commit-bot: I haz the power, 2017-05-12 18:43:02]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b06d81fdf9fc3a85fe95aa3ef9cd...