Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890643003:
Add bench for image encodes (Closed)
DescriptionAdd bench for image encodes
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1890643003
Committed: https://skia.googlesource.com/skia/+/b92dc7ac0bd4109411851d7d3684ed3aa7108086
Patch Set 1 #
Total comments: 7
Patch Set 2 : Response to comments #
Dependent Patchsets: Messages
Total messages: 12 (4 generated)
Description was changed from ========== Add bench for image encodes BUG=skia: ========== to ========== Add bench for image encodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + scroggo@google.com
Skia's crude RGB -> YUV conversion seems to be causing a few bugs in Android. https://buganizer.corp.google.com/u/0/issues/28161384 https://buganizer.corp.google.com/u/0/issues/21891795 We should be fine to let libjpeg-turbo handle the RGB -> YUV conversion. I would guess that this is faster and more accurate. I want to go ahead and add a benchmark though, so we can verify.
lgtm https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp File bench/EncoderBench.cpp (right): https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:64: SkImageEncoder::Type fType; nit: Many of these variables could be const. https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:72: DEF_BENCH(return new EncodeBench("mandrill_512.png", SkImageEncoder::kJPEG_Type, 90)); Would it be interesting to use more than one image? Maybe that should be a TODO? https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:75: DEF_BENCH(return new EncodeBench("mandrill_512.png", SkImageEncoder::kWEBP_Type, 90)); Do they also use 90 for quality on WEBP encodes? Do they use WEBP to encode?
https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp File bench/EncoderBench.cpp (right): https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:64: SkImageEncoder::Type fType; On 2016/04/14 13:43:59, scroggo wrote: > nit: Many of these variables could be const. Done. https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:72: DEF_BENCH(return new EncodeBench("mandrill_512.png", SkImageEncoder::kJPEG_Type, 90)); On 2016/04/14 13:43:59, scroggo wrote: > Would it be interesting to use more than one image? Maybe that should be a TODO? I think so. I don't quite understand it well enough to know what types of images might be interesting though. It's easy enough to add another image here. I'll go ahead and do that. Or do you think we should run this on a whole set of images like we do for CodecBench? If we do that, I think we should add a separate directory on gsutil for images to test encoding with (rather than test on ALL of them). https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:75: DEF_BENCH(return new EncodeBench("mandrill_512.png", SkImageEncoder::kWEBP_Type, 90)); On 2016/04/14 13:43:59, scroggo wrote: > Do they also use 90 for quality on WEBP encodes? Do they use WEBP to encode? The jpeg info is from judds' comment on the bug: Background: A big part of the reason why Glide makes image loading feel fast is that Glide caches downsampled and cropped images to disk (JPEG, quality 90). We've had a few reports of quality issues (usually tinting and banding) for these re-compressed images (https://github.com/bumptech/glide/issues/1069, https://github.com/bumptech/glide/issues/515, b/21891795). https://buganizer.corp.google.com/u/0/issues/28161384 I doubt that they use webp encoding, though I'm not sure. I'm adding a TODO to find the appropriate quality mode to bench webp encodes.
Still lgtm, with a suggestion that maybe it would be more relevant to use a JPEG captured from a camera. (I think these came from Photoshop or something?) https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp File bench/EncoderBench.cpp (right): https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... bench/EncoderBench.cpp:72: DEF_BENCH(return new EncodeBench("mandrill_512.png", SkImageEncoder::kJPEG_Type, 90)); On 2016/04/18 17:25:06, msarett wrote: > On 2016/04/14 13:43:59, scroggo wrote: > > Would it be interesting to use more than one image? Maybe that should be a > TODO? > > I think so. I don't quite understand it well enough to know what types of > images might be interesting though. > > It's easy enough to add another image here. I'll go ahead and do that. Or do > you think we should run this on a whole set of images like we do for CodecBench? I'm not sure. I definitely think this is a good start. To answer that question, I think we really need to learn more about encoding. What makes it take a different path? I could imagine caring about alpha (since we may have premultiplied the alpha, which is not what an encoder wants), but that is not relevant for encoding to JPEG. Using different images seems somewhat misleading. For example, we might have two images that look (more or less) the same but are encoded differently. Once we decode them to SkImages, encoding them to JPEG should be the same. So what might be more interesting is to just draw (exercising features that are relevant) and time the encode. But given that Photos (the app that really cares here) is starting with JPEGs, using a JPEG is a reasonable thing to start with. Maybe we should be starting with an actual camera photo? > If we do that, I think we should add a separate directory on gsutil for images > to test encoding with (rather than test on ALL of them).
On 2016/04/18 17:53:49, scroggo wrote: > Still lgtm, with a suggestion that maybe it would be more relevant to use a JPEG > captured from a camera. (I think these came from Photoshop or something?) > > https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp > File bench/EncoderBench.cpp (right): > > https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... > bench/EncoderBench.cpp:72: DEF_BENCH(return new EncodeBench("mandrill_512.png", > SkImageEncoder::kJPEG_Type, 90)); > On 2016/04/18 17:25:06, msarett wrote: > > On 2016/04/14 13:43:59, scroggo wrote: > > > Would it be interesting to use more than one image? Maybe that should be a > > TODO? > > > > I think so. I don't quite understand it well enough to know what types of > > images might be interesting though. > > > > It's easy enough to add another image here. I'll go ahead and do that. Or do > > you think we should run this on a whole set of images like we do for > CodecBench? > > I'm not sure. I definitely think this is a good start. To answer that question, > I think we really need to learn more about encoding. What makes it take a > different path? I could imagine caring about alpha (since we may have > premultiplied the alpha, which is not what an encoder wants), but that is not > relevant for encoding to JPEG. > > Using different images seems somewhat misleading. For example, we might have two > images that look (more or less) the same but are encoded differently. Once we > decode them to SkImages, encoding them to JPEG should be the same. So what might > be more interesting is to just draw (exercising features that are relevant) and > time the encode. But given that Photos (the app that really cares here) is > starting with JPEGs, using a JPEG is a reasonable thing to start with. Maybe we > should be starting with an actual camera photo? > > > If we do that, I think we should add a separate directory on gsutil for > images > > to test encoding with (rather than test on ALL of them). All good suggestions, I think. I'll land for now, and intend to circle back if/when encoding become a priority.
On 2016/04/18 17:59:40, msarett wrote: > On 2016/04/18 17:53:49, scroggo wrote: > > Still lgtm, with a suggestion that maybe it would be more relevant to use a > JPEG > > captured from a camera. (I think these came from Photoshop or something?) > > > > https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp > > File bench/EncoderBench.cpp (right): > > > > > https://codereview.chromium.org/1890643003/diff/1/bench/EncoderBench.cpp#newc... > > bench/EncoderBench.cpp:72: DEF_BENCH(return new > EncodeBench("mandrill_512.png", > > SkImageEncoder::kJPEG_Type, 90)); > > On 2016/04/18 17:25:06, msarett wrote: > > > On 2016/04/14 13:43:59, scroggo wrote: > > > > Would it be interesting to use more than one image? Maybe that should be a > > > TODO? > > > > > > I think so. I don't quite understand it well enough to know what types of > > > images might be interesting though. > > > > > > It's easy enough to add another image here. I'll go ahead and do that. Or > do > > > you think we should run this on a whole set of images like we do for > > CodecBench? > > > > I'm not sure. I definitely think this is a good start. To answer that > question, > > I think we really need to learn more about encoding. What makes it take a > > different path? I could imagine caring about alpha (since we may have > > premultiplied the alpha, which is not what an encoder wants), but that is not > > relevant for encoding to JPEG. > > > > Using different images seems somewhat misleading. For example, we might have > two > > images that look (more or less) the same but are encoded differently. Once we > > decode them to SkImages, encoding them to JPEG should be the same. So what > might > > be more interesting is to just draw (exercising features that are relevant) > and > > time the encode. But given that Photos (the app that really cares here) is > > starting with JPEGs, using a JPEG is a reasonable thing to start with. Maybe > we > > should be starting with an actual camera photo? > > > > > If we do that, I think we should add a separate directory on gsutil for > > images > > > to test encoding with (rather than test on ALL of them). > > All good suggestions, I think. I'll land for now, and intend to circle back > if/when encoding become a priority. sgtm
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/1890643003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890643003/20001
Message was sent while issue was closed.
Description was changed from ========== Add bench for image encodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add bench for image encodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/b92dc7ac0bd4109411851d7d3684ed3aa7108086 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/b92dc7ac0bd4109411851d7d3684ed3aa7108086 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
