Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1983853002:
Update libpng to 1.6.22rc01 (Closed)
DescriptionUpdate libpng to 1.6.22rc01
If there are no issues, this version will be released May 26.
Let's start testing with it, so we are ready to update Chrome when
it is available.
BUG=skia:4710
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1983853002
Committed: https://skia.googlesource.com/skia/+/8fe89ab9b2c0089451d373c9ea76245fc8ef5a45
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Added README.google and updated a bug #Messages
Total messages: 29 (13 generated)
Description was changed from ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be release May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia: ========== to ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be release May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be release May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be released May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1983853002/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/1983853002/diff/20001/DEPS#newcode21 DEPS:21: "third_party/externals/libpng" : "https://github.com/mattsarett/libpng.git@393063051764f2410daf931e8f414f37e157f7a1", It's strange to clone from github/mattsarett. We can't use the libpng mirror because there is now one extra config step. We need to run the following command to enable the Intel optimizations. patch -i contrib/intel/intel_sse.patch -p1
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1983853002/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/1983853002/diff/20001/DEPS#newcode21 DEPS:21: "third_party/externals/libpng" : "https://github.com/mattsarett/libpng.git@393063051764f2410daf931e8f414f37e157f7a1", On 2016/05/16 14:14:16, msarett wrote: > It's strange to clone from github/mattsarett. > > We can't use the libpng mirror because there is now one extra config step. We > need to run the following command to enable the Intel optimizations. > > patch -i contrib/intel/intel_sse.patch -p1 So does this mean we'll have to continue to use the github/mattsarett version? I'm surprised that libpng is making us do a patch to enable these. It seems like it would be cleaner to use a build flag? What does the patch look like*? Can we instead add something to third_party/libpng/libpngconf.h [1]? [1] https://skia.googlesource.com/skia/+/master/third_party/libpng/pnglibconf.h I just answered my own question. It can be found here: [2]. It does the following: - Update configure.ac andMakefile.am - We don't use either of these, so do we need these changes? - Modify pngpriv.h - Can we instead modify our pnglibconf.h? It looks like pngpriv.h includes it? [2] https://skia.googlesource.com/third_party/libpng/+/v1.6.22rc01/contrib/intel/...
The reason this is "off by default" is that libpng didn't like the Google copyright headers. I'll see if I can make this work doing everything from pnglibconf.h The eventual plan is to mirror AOSP (or I guess chrome), so the change to github.com/mattsarett would have been temporary.
On 2016/05/16 19:20:45, scroggo wrote: > https://codereview.chromium.org/1983853002/diff/20001/DEPS > File DEPS (right): > > https://codereview.chromium.org/1983853002/diff/20001/DEPS#newcode21 > DEPS:21: "third_party/externals/libpng" : > "https://github.com/mattsarett/libpng.git@393063051764f2410daf931e8f414f37e157f7a1", > On 2016/05/16 14:14:16, msarett wrote: > > It's strange to clone from github/mattsarett. > > > > We can't use the libpng mirror because there is now one extra config step. We > > need to run the following command to enable the Intel optimizations. > > > > patch -i contrib/intel/intel_sse.patch -p1 > > So does this mean we'll have to continue to use the github/mattsarett version? > I'm surprised that libpng is making us do a patch to enable these. It seems like > it would be cleaner to use a build flag? What does the patch look like*? Can we > instead add something to third_party/libpng/libpngconf.h [1]? > > [1] https://skia.googlesource.com/skia/+/master/third_party/libpng/pnglibconf.h > > I just answered my own question. It can be found here: [2]. It does the > following: > - Update configure.ac andMakefile.am > - We don't use either of these, so do we need these changes? > - Modify pngpriv.h > - Can we instead modify our pnglibconf.h? It looks like pngpriv.h includes it? > > [2] > https://skia.googlesource.com/third_party/libpng/+/v1.6.22rc01/contrib/intel/... Unfortunately, the function declarations depend on internal libpng types that aren't exposed publicly... We need modifications to pngpriv.h. Maybe we should view this update as "temporary in order to test the new library for Chrome". Then we can either revert or switch our mirror to chrome after. We did something similar with github/mtklein when we were testing the Intel opts.
Add BUG=skia:4573 to the CL description? On 2016/05/16 19:28:00, msarett wrote: > The reason this is "off by default" is that libpng didn't like the Google > copyright headers. That's a shame. So general clients won't see the benefit of your patches? https://codereview.chromium.org/1983853002/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/1983853002/diff/20001/DEPS#newcode21 DEPS:21: "third_party/externals/libpng" : "https://github.com/mattsarett/libpng.git@393063051764f2410daf931e8f414f37e157f7a1", On 2016/05/16 19:20:45, scroggo wrote: > On 2016/05/16 14:14:16, msarett wrote: > > It's strange to clone from github/mattsarett. > > > > We can't use the libpng mirror because there is now one extra config step. We > > need to run the following command to enable the Intel optimizations. > > > > patch -i contrib/intel/intel_sse.patch -p1 > > So does this mean we'll have to continue to use the github/mattsarett version? > I'm surprised that libpng is making us do a patch to enable these. It seems like > it would be cleaner to use a build flag? What does the patch look like*? Can we > instead add something to third_party/libpng/libpngconf.h [1]? > > [1] https://skia.googlesource.com/skia/+/master/third_party/libpng/pnglibconf.h > > I just answered my own question. It can be found here: [2]. It does the > following: > - Update configure.ac andMakefile.am > - We don't use either of these, so do we need these changes? > - Modify pngpriv.h > - Can we instead modify our pnglibconf.h? It looks like pngpriv.h includes it? > > [2] > https://skia.googlesource.com/third_party/libpng/+/v1.6.22rc01/contrib/intel/... Matt said: > Unfortunately, the function declarations depend on internal libpng types that > aren't exposed publicly... We need modifications to pngpriv.h. :( My preference would be to update libpng to not require a patch, but I guess we cannot have that for this version no matter what. Please document in this file why we're pulling from your repository and what we need to do to stop (and/or file a bug). We should also have a README.google describing what our modifications are (and why). Maybe that belongs in your repository, but perhaps we should also modify the README we have checked into third_party/libpng [1]. [1] https://skia.googlesource.com/skia/+/master/third_party/libpng/README.google
Description was changed from ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be released May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be released May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia:4710 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
On 2016/05/16 21:16:10, scroggo wrote: > Add BUG=skia:4573 to the CL description? > > On 2016/05/16 19:28:00, msarett wrote: > > The reason this is "off by default" is that libpng didn't like the Google > > copyright headers. > > That's a shame. So general clients won't see the benefit of your patches? Depends on what you mean by general clients, but yes, they have to explicitly turn the opts on. > > https://codereview.chromium.org/1983853002/diff/20001/DEPS > File DEPS (right): > > https://codereview.chromium.org/1983853002/diff/20001/DEPS#newcode21 > DEPS:21: "third_party/externals/libpng" : > "https://github.com/mattsarett/libpng.git@393063051764f2410daf931e8f414f37e157f7a1", > On 2016/05/16 19:20:45, scroggo wrote: > > On 2016/05/16 14:14:16, msarett wrote: > > > It's strange to clone from github/mattsarett. > > > > > > We can't use the libpng mirror because there is now one extra config step. > We > > > need to run the following command to enable the Intel optimizations. > > > > > > patch -i contrib/intel/intel_sse.patch -p1 > > > > So does this mean we'll have to continue to use the github/mattsarett version? > > I'm surprised that libpng is making us do a patch to enable these. It seems > like > > it would be cleaner to use a build flag? What does the patch look like*? Can > we > > instead add something to third_party/libpng/libpngconf.h [1]? > > > > [1] > https://skia.googlesource.com/skia/+/master/third_party/libpng/pnglibconf.h > > > > I just answered my own question. It can be found here: [2]. It does the > > following: > > - Update configure.ac andMakefile.am > > - We don't use either of these, so do we need these changes? > > - Modify pngpriv.h > > - Can we instead modify our pnglibconf.h? It looks like pngpriv.h includes > it? > > > > [2] > > > https://skia.googlesource.com/third_party/libpng/+/v1.6.22rc01/contrib/intel/... > > Matt said: > > Unfortunately, the function declarations depend on internal libpng types that > > aren't exposed publicly... We need modifications to pngpriv.h. > > :( > > My preference would be to update libpng to not require a patch, but I guess we > cannot have that for this version no matter what. > When we were upstreaming, I tried very hard for this. And the beta version that we currently mirror is designed this way. In the end, it was moved out into a patch because: (1) libpng refused to put "Copyright Google" in its core files. (2) Google refused to transfer the copyright to libpng. > Please document in this file why we're pulling from your repository and what we > need to do to stop (and/or file a bug). > I updated skbug.com/4710 > We should also have a README.google describing what our modifications are (and > why). Maybe that belongs in your repository, but perhaps we should also modify > the README we have checked into third_party/libpng [1]. > > [1] https://skia.googlesource.com/skia/+/master/third_party/libpng/README.google Added a README.google to my repository and updated the README.google checked into Skia already.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983853002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983853002/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983853002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983853002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983853002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983853002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983853002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983853002/40001
Message was sent while issue was closed.
Description was changed from ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be released May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia:4710 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Update libpng to 1.6.22rc01 If there are no issues, this version will be released May 26. Let's start testing with it, so we are ready to update Chrome when it is available. BUG=skia:4710 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8fe89ab9b2c0089451d373c9ea76245fc8ef5a45 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/8fe89ab9b2c0089451d373c9ea76245fc8ef5a45 |
