Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by msarett Modified:
4 years, 10 months ago Reviewers:
scroggo, mtklein CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdding a tool to get images from skps
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1696763002
Committed: https://skia.googlesource.com/skia/+/3478f753ffc28a6f0f0877cc06be7373f960c527
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Response to comments #
Total comments: 1
Patch Set 3 : Track unknown images #
Created: 4 years, 10 months ago
Messages
Total messages: 18 (7 generated)
Description was changed from ========== Adding a tool to get from images from skps BUG=skia: ========== to ========== Adding a tool to get from images from skps 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
msarett@google.com changed reviewers: + scroggo@google.com
Once this tool (or something like it) is ready, I'm going to talk to Ravi about generating a new set of skps (without reencoding any images). I've run this on a small set of skps to test. So far I'm a little surprised that all of the images seem quite small. https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... File tools/get_images_from_skps.cpp (right): https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:130: SkAutoTUnref<SkPicture> picture(SkPicture::CreateFromStream(stream, store_encoded_to_file)); Originally, I wanted to write code that actually parsed the SkPicture. But I think this is a nice way to take advantage of the fact that someone has already written that code :).
https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... File tools/get_images_from_skps.cpp (right): https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:24: SkDebugf("Usage: get_images_from_skps -s <dir of skps> -o <dir for output images>\n"); SkCommandLineFlags has a method to specify usage (SetUsage), which gets printed when you use -h/--help. Maybe we should add a method to print that usage, and you can use that? Then this message will also be printed when you use -h. https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:31: path.append("/"); Should these use SkOSPath::Join so that it will work on Windows? https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:130: SkAutoTUnref<SkPicture> picture(SkPicture::CreateFromStream(stream, store_encoded_to_file)); On 2016/02/12 16:33:49, msarett wrote: > Originally, I wanted to write code that actually parsed the SkPicture. But I > think this is a nice way to take advantage of the fact that someone has already > written that code :).RWBuffer_threadSafe, Yep! This is similar to a feature we used to have in the long dead render_pictures program, which was subsumed into DM. (Although I just did a git grep of "render_pictures" and it turns out we have two python scripts that use it, plus some documentation that refers to it. So we have more cleanups to do there...) Why not put this inside DM also? https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:136: SkDebugf("Saved %d images to %s\n", gSuccessCtr, gOutputDir); Should we also report the number of images that failed? https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:137: return 1; FWIW, I think our other testing tools return 0 and success and 1 on failure.
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... File tools/get_images_from_skps.cpp (right): https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:130: SkAutoTUnref<SkPicture> picture(SkPicture::CreateFromStream(stream, store_encoded_to_file)); > Why not put this inside DM also? Unlike render_pictures, this program has multiple outputs per input, which doesn't fit well into DM's model. Let's just leave it stand-alone.
lgtm https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... File tools/get_images_from_skps.cpp (right): https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:130: SkAutoTUnref<SkPicture> picture(SkPicture::CreateFromStream(stream, store_encoded_to_file)); On 2016/02/12 17:21:41, mtklein wrote: > > Why not put this inside DM also? > > Unlike render_pictures, this program has multiple outputs per input, which > doesn't fit well into DM's model. Let's just leave it stand-alone. sgtm
Using SkOSPath::Join() and added a SkCommandLineFlags::PrintUsage(). https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... File tools/get_images_from_skps.cpp (right): https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:24: SkDebugf("Usage: get_images_from_skps -s <dir of skps> -o <dir for output images>\n"); On 2016/02/12 17:18:38, scroggo wrote: > SkCommandLineFlags has a method to specify usage (SetUsage), which gets printed > when you use -h/--help. Maybe we should add a method to print that usage, and > you can use that? Then this message will also be printed when you use -h. Done. https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:31: path.append("/"); On 2016/02/12 17:18:38, scroggo wrote: > Should these use SkOSPath::Join so that it will work on Windows? Done. https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:136: SkDebugf("Saved %d images to %s\n", gSuccessCtr, gOutputDir); On 2016/02/12 17:18:38, scroggo wrote: > Should we also report the number of images that failed? Yes that sounds good. https://codereview.chromium.org/1696763002/diff/20001/tools/get_images_from_s... tools/get_images_from_skps.cpp:137: return 1; On 2016/02/12 17:18:38, scroggo wrote: > FWIW, I think our other testing tools return 0 and success and 1 on failure. Haha yes. Fixed.
Description was changed from ========== Adding a tool to get from images from skps BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Adding a tool to get images from skps BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1696763002/diff/40001/tools/get_images_from_s... File tools/get_images_from_skps.cpp (right): https://codereview.chromium.org/1696763002/diff/40001/tools/get_images_from_s... tools/get_images_from_skps.cpp:98: gFailureCtr++; I guess "number of failures" is ambiguous - I was thinking the images for which we did not create a codec (or the "default" case). This number tells us the number of times the file system gave us trouble, which may be interesting (the data we retrieved is not complete). But I think failures to create codec (of one of the types we expect) is a sign of a deeper problem.
Hmmm yeah. I think I'll break up successes into "known" and "unknown" images.
Now keeps track of recognized and unrecognized images.
lgtm
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/1696763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1696763002/60001
Message was sent while issue was closed.
Description was changed from ========== Adding a tool to get images from skps BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Adding a tool to get images from skps 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/+/3478f753ffc28a6f0f0877cc06be7373f960c527 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://skia.googlesource.com/skia/+/3478f753ffc28a6f0f0877cc06be7373f960c527 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
