New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 724616 link

Starred by 1 user
Status: Fixed
Owner:
█ liyuqian@chromium.org
Last visit > 30 days ago
Closed: Aug 2017
Cc:
noel@chromium.org
scroggo@chromium.org
█ e...@chromium.org
dcheng@chromium.org
halcanary@chromium.org
█ msarett@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Use Skia encoders in ui/gfx

Project Member Reported by msarett@chromium.org, May 19 2017 
The Webkit platform is in the process of switching over to use Skia's image encoders.  This is motivated by a need for color space support, and also seems like a generally positive refactor.
https://bugs.chromium.org/p/chromium/issues/detail?id=713862

I think it makes sense to go ahead and use Skia's encoders in ui/gfx/codec as well.  Any thoughts?

Comment 1 by dcheng@chromium.org, May 19 2017

Cc: noel@chromium.org
noel@ mentioned that we're trying to do this in Blink already. A few questions that spring to mind:
- Does skia use libpng, libjpeg-turbo, etc under the hood?
- Where is the documentation for the Skia codecs? For the current libraries that we use, we can get (some semblance of) documentation via search and man pages.
- Will the API surface exposed to consumers of change?

Comment 2 by msarett@chromium.org, May 19 2017 

- Does skia use libpng, libjpeg-turbo, etc under the hood?

Yes.

- Where is the documentation for the Skia codecs? For the current libraries that we use, we can get (some semblance of) documentation via search and man pages.

https://cs.chromium.org/chromium/src/third_party/skia/include/encode/SkJpegEncoder.h
https://cs.chromium.org/chromium/src/third_party/skia/include/encode/SkPngEncoder.h

- Will the API surface exposed to consumers of change?

I think it would make sense for the API surface to stay the same.  The current API would wrap the Skia encoders.

Comment 3 by msarett@chromium.org, May 22 2017 

To follow-up, I've take a closer look at how the hypothetical CL would look for jpeg.  I think I would propose a minor API change.

It would make sense to use SkColorType in place of ColorFormat to describe the input.  Since the underlying encoder supports all logical SkColorTypes, it makes sense to expose this in the API.  Also, if we take ColorFormat, we will need to convert it to SkColorType anyway.

Comment 4 by msarett@chromium.org, May 24 2017 

I'm going to go ahead and push forward on this.  I think this is good for code consolidation and consistency with the platform.

One noteworthy change will be the removal of the input FORMAT_RGB.  It is only used in unit tests and does not have a corresponding SkColorType.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 5 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84202c54588f62531bf548aecc6a313bb7a56b39

commit 84202c54588f62531bf548aecc6a313bb7a56b39
Author: stevenjb <stevenjb@chromium.org>
Date: Mon Jun 05 23:08:19 2017

Revert of Delete FORMAT_RGB and legacy libjpeg support from gfx::JpegCodec (patchset #2 id:20001 of https://codereview.chromium.org/2920263002/ )

Reason for revert:
This broke the PFQ chrome build:

https://build.chromium.org/p/chromiumos.chromium/builders/amd64-generic-tot-asan-informational/builds/13268

Original issue's description:
> Delete FORMAT_RGB and legacy libjpeg support from gfx::JpegCodec
>
> This was split from:
> https://codereview.chromium.org/2895953003/
>
> BUG= 724616 
>
> Review-Url: https://codereview.chromium.org/2920263002
> Cr-Commit-Position: refs/heads/master@{#477057}
> Committed: https://chromium.googlesource.com/chromium/src/+/5f026c277b1c0945bb99b2ffcfcc76f9d10622b2

TBR=scroggo@chromium.org,dcheng@chromium.org,msarett@chromium.org,ihf@chromium.org,msarett@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 724616 

Review-Url: https://codereview.chromium.org/2924733002
Cr-Commit-Position: refs/heads/master@{#477114}

[modify] https://crrev.com/84202c54588f62531bf548aecc6a313bb7a56b39/ui/gfx/codec/jpeg_codec.cc
[modify] https://crrev.com/84202c54588f62531bf548aecc6a313bb7a56b39/ui/gfx/codec/jpeg_codec.h
[modify] https://crrev.com/84202c54588f62531bf548aecc6a313bb7a56b39/ui/gfx/codec/jpeg_codec_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a9cbb594d5b28bdc51b7b505d201930221ddbe8a

commit a9cbb594d5b28bdc51b7b505d201930221ddbe8a
Author: msarett <msarett@google.com>
Date: Tue Jun 13 20:19:29 2017

Remove FORMAT_RGB from gfx::PngCodec

This is only used in test code.

BUG= 724616 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2927893002
Cr-Commit-Position: refs/heads/master@{#479124}

[modify] https://crrev.com/a9cbb594d5b28bdc51b7b505d201930221ddbe8a/media/gpu/rendering_helper.cc
[modify] https://crrev.com/a9cbb594d5b28bdc51b7b505d201930221ddbe8a/media/gpu/rendering_helper.h
[modify] https://crrev.com/a9cbb594d5b28bdc51b7b505d201930221ddbe8a/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/a9cbb594d5b28bdc51b7b505d201930221ddbe8a/ui/gfx/codec/png_codec.cc
[modify] https://crrev.com/a9cbb594d5b28bdc51b7b505d201930221ddbe8a/ui/gfx/codec/png_codec.h
[modify] https://crrev.com/a9cbb594d5b28bdc51b7b505d201930221ddbe8a/ui/gfx/codec/png_codec_unittest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/61479a854982e3749c4322a4c5dede07b713a163

commit 61479a854982e3749c4322a4c5dede07b713a163
Author: owenlin <owenlin@chromium.org>
Date: Thu Jun 15 11:56:54 2017

Revert of Remove FORMAT_RGB from gfx::PngCodec (patchset #3 id:40001 of https://codereview.chromium.org/2927893002/ )

Reason for revert:
Break the VDA unittest, see  http://crbug.com/733522 .

Original issue's description:
> Remove FORMAT_RGB from gfx::PngCodec
>
> This is only used in test code.
>
> BUG= 724616 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
>
> Review-Url: https://codereview.chromium.org/2927893002
> Cr-Commit-Position: refs/heads/master@{#479124}
> Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201930221ddbe8a

TBR=msarett@chromium.org,scroggo@chromium.org,dcheng@chromium.org,sandersd@chromium.org,msarett@google.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 724616 

Review-Url: https://codereview.chromium.org/2943433002
Cr-Commit-Position: refs/heads/master@{#479669}

[modify] https://crrev.com/61479a854982e3749c4322a4c5dede07b713a163/media/gpu/rendering_helper.cc
[modify] https://crrev.com/61479a854982e3749c4322a4c5dede07b713a163/media/gpu/rendering_helper.h
[modify] https://crrev.com/61479a854982e3749c4322a4c5dede07b713a163/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/61479a854982e3749c4322a4c5dede07b713a163/ui/gfx/codec/png_codec.cc
[modify] https://crrev.com/61479a854982e3749c4322a4c5dede07b713a163/ui/gfx/codec/png_codec.h
[modify] https://crrev.com/61479a854982e3749c4322a4c5dede07b713a163/ui/gfx/codec/png_codec_unittest.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7060f1277a34592bc1f6c16762ccb0345669b073

commit 7060f1277a34592bc1f6c16762ccb0345669b073
Author: msarett <msarett@google.com>
Date: Thu Jun 15 14:34:53 2017

Use SkJpegEncoder in gfx jpeg_codec

TBR=jochen@chromium.org
TBR=reveman@chromium.org

BUG= 724616 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2895953003
Cr-Commit-Position: refs/heads/master@{#479694}

[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/cc/debug/picture_debug_util.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_test_utils.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/chrome/renderer/chrome_render_frame_observer.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/components/suggestions/image_encoder.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/components/user_manager/user_image/user_image.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/components/wallpaper/wallpaper_manager_base.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/content/browser/devtools/devtools_frame_trace_recorder.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/extensions/browser/api/web_contents_capture_client.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/media/capture/video/fake_video_capture_device.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/media/gpu/jpeg_decode_accelerator_unittest.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/pdf/pdfium/pdfium_engine.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/services/data_decoder/image_decoder_impl_unittest.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/ui/gfx/codec/jpeg_codec.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/ui/gfx/codec/jpeg_codec.h
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/ui/gfx/codec/jpeg_codec_unittest.cc
[modify] https://crrev.com/7060f1277a34592bc1f6c16762ccb0345669b073/ui/gfx/image/image_util.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64759b2876675ce91e2049cc2658b8c8068ec6c8

commit 64759b2876675ce91e2049cc2658b8c8068ec6c8
Author: msarett <msarett@google.com>
Date: Fri Jun 16 12:21:00 2017

Remove FORMAT_RGB from gfx::PngCodec

This is only used in test code.

BUG= 724616 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2927893002
Cr-Original-Commit-Position: refs/heads/master@{#479124}
Committed: https://chromium.googlesource.com/chromium/src/+/a9cbb594d5b28bdc51b7b505d201930221ddbe8a
Review-Url: https://codereview.chromium.org/2927893002
Cr-Commit-Position: refs/heads/master@{#480024}

[modify] https://crrev.com/64759b2876675ce91e2049cc2658b8c8068ec6c8/media/gpu/rendering_helper.cc
[modify] https://crrev.com/64759b2876675ce91e2049cc2658b8c8068ec6c8/media/gpu/rendering_helper.h
[modify] https://crrev.com/64759b2876675ce91e2049cc2658b8c8068ec6c8/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/64759b2876675ce91e2049cc2658b8c8068ec6c8/ui/gfx/codec/png_codec.cc
[modify] https://crrev.com/64759b2876675ce91e2049cc2658b8c8068ec6c8/ui/gfx/codec/png_codec.h
[modify] https://crrev.com/64759b2876675ce91e2049cc2658b8c8068ec6c8/ui/gfx/codec/png_codec_unittest.cc

Comment 12 by msarett@chromium.org, Jun 16 2017

Cc: msarett@chromium.org halcanary@chromium.org
Owner: liyuqian@chromium.org
Last step is to switch over to SkPngEncoder.  We should be clear to do that now.  Passing to liyuqian@.

Comment 14 by liyuqian@chromium.org, Jul 25 2017 

Should I consider this to be fixed?

Comment 15 by scroggo@chromium.org, Aug 7 2017

Status: Fixed (was: Assigned)
Yes. Marking as Fixed.
►

Sign in to add a comment