Issue 6300056: A very rough first past at implementing a test program for skia commands. (Closed)
DescriptionProgram to test serialized SkPictures.
I created a program to that will take a directory full of serialized SkPictures and then render the images to a specified directory.
Comitted: http://code.google.com/p/skia/source/detail?r=4235
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #
MessagesTotal messages: 19
https://codereview.appspot.com/6300056/diff/1/Makefile File Makefile (right): https://codereview.appspot.com/6300056/diff/1/Makefile#newcode32 Makefile:32: ALL_TARGETS := core SampleApp bench gm tests tools run_command_test Don't like this name. Try to keep it short, yet meaningful. How about "picture" https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.cpp File command_tests/run_command_test.cpp (right): https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:18: #include<iostream> AFAIK, we do not use STL in skia. Please don't add that new dependency. Beside, I don't even see where you are using anything from this header. https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:27: // Why yes, the UI is terrible Unnecessary comment. The UI is fine: it is dev friendly and script friendly. https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:29: SkDebugf("%s input_dir [expected_dir [output_dir]]\n", argv0); Perhaps a short description of what these directories are expected to contain. https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:32: static void setup_bitmap(//const ConfigData& gRec, SkISize& size, Cleanup commented-out code. https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:35: bitmap->setConfig(SkBitmap::kARGB_8888_Config, 800, 600); Eventually we should find a way to encode width and height in the picture. https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:66: static inline SkPMColor compute_diff_pmcolor(SkPMColor c0, SkPMColor c1) { Looks like there is an opportunity to share code between test programs. Perhaps you could move shared code to the utils directory. Something like SkTestImageUtils.h/cpp https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:198: generate_image_from_picture(/*gm, gRec, */&picture, &actual); more comment noise here https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:279: } ExpectedDir is an optional command line argument, but it looks like this program does nothing if an expectedDir is not specified. Seems weird. https://codereview.appspot.com/6300056/diff/1/gyp/command_tests.gyp File gyp/command_tests.gyp (right): https://codereview.appspot.com/6300056/diff/1/gyp/command_tests.gyp#newcode18 gyp/command_tests.gyp:18: # 'effects.gyp:effects', Remove these lines if you don't need them.
Sign in to reply to this message.
https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.cpp File command_tests/run_command_test.cpp (right): https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... command_tests/run_command_test.cpp:66: static inline SkPMColor compute_diff_pmcolor(SkPMColor c0, SkPMColor c1) { On 2012/06/07 13:47:45, junov1 wrote: > Looks like there is an opportunity to share code between test programs. > Perhaps you could move shared code to the utils directory. Something like > SkTestImageUtils.h/cpp Should we just write the images to a file and compare with skdiff?
Sign in to reply to this message.
On 2012/06/07 13:52:30, bsalomon wrote: > https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.cpp > File command_tests/run_command_test.cpp (right): > > https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... > command_tests/run_command_test.cpp:66: static inline SkPMColor > compute_diff_pmcolor(SkPMColor c0, SkPMColor c1) { > On 2012/06/07 13:47:45, junov1 wrote: > > Looks like there is an opportunity to share code between test programs. > > Perhaps you could move shared code to the utils directory. Something like > > SkTestImageUtils.h/cpp > > Should we just write the images to a file and compare with skdiff? That seems like a much better idea. I will do that.
Sign in to reply to this message.
On 2012/06/07 15:12:09, keyar wrote: > On 2012/06/07 13:52:30, bsalomon wrote: > > > https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.cpp > > File command_tests/run_command_test.cpp (right): > > > > > https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... > > command_tests/run_command_test.cpp:66: static inline SkPMColor > > compute_diff_pmcolor(SkPMColor c0, SkPMColor c1) { > > On 2012/06/07 13:47:45, junov1 wrote: > > > Looks like there is an opportunity to share code between test programs. > > > Perhaps you could move shared code to the utils directory. Something like > > > SkTestImageUtils.h/cpp > > > > Should we just write the images to a file and compare with skdiff? > > That seems like a much better idea. I will do that. Yeah, that should do it. Let me know if you need any additions to skdiff for the correctness portion, but.
Sign in to reply to this message.
I don't think skdiff will need to be modified. The current idea is to write a program that will render the SkPictures to file and then use skdiff. A python script would then be used to glue these two steps together. On 2012/06/07 15:13:00, TomH wrote: > On 2012/06/07 15:12:09, keyar wrote: > > On 2012/06/07 13:52:30, bsalomon wrote: > > > > > > https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.cpp > > > File command_tests/run_command_test.cpp (right): > > > > > > > > > https://codereview.appspot.com/6300056/diff/1/command_tests/run_command_test.... > > > command_tests/run_command_test.cpp:66: static inline SkPMColor > > > compute_diff_pmcolor(SkPMColor c0, SkPMColor c1) { > > > On 2012/06/07 13:47:45, junov1 wrote: > > > > Looks like there is an opportunity to share code between test programs. > > > > Perhaps you could move shared code to the utils directory. Something like > > > > SkTestImageUtils.h/cpp > > > > > > Should we just write the images to a file and compare with skdiff? > > > > That seems like a much better idea. I will do that. > > Yeah, that should do it. Let me know if you need any additions to skdiff for the > correctness portion, but.
Sign in to reply to this message.
I uploaded a new patch. Right now it is only for the program that renders in serialized SkPictures. I still need to create the python glue script.
Sign in to reply to this message.
Go ahead and leave in the .skp restriction; we can relax it later if we need to. skdiff is actually a tiny bit too relaxed right now, tries to parse .html and then complains about it. https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.cpp File tools/render_pictures_main.cpp (right): https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:28: " outputDir: directory to write the rendered SkPictures.\n"); Nit: description of outputDir is not quite clear https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:35: if (len > 0 && dir[len - 1] != '/') { Hey, Brian: is it just Ganesh that puts the constant on the right of the conditional, or does Skia do everything Yoda-style, too? https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:41: static void open_picture_stream(const char* inputDir, SkString& inputFilename, Nit: inputFilename can be declared const, can't it? https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:42: SkFILEStream& inputStream) { Skia style is to pass a pointer to non-const stuff like inputStream, so that it's clearer in the calling context that you're passing something that may change. (And yeah, using char* instead of a string class makes this slightly inconsistent...) https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:58: static void setup_bitmap(SkPicture& picture, SkBitmap& bitmap) { SkBitmap* bitmap https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:65: static void generate_image_from_picture(SkPicture& pict, SkBitmap& bitmap) { Ditto https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:103: SkString& inputFilename) { const?
Sign in to reply to this message.
https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.cpp File tools/render_pictures_main.cpp (right): https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:35: if (len > 0 && dir[len - 1] != '/') { On 2012/06/07 19:44:53, TomH wrote: > Hey, Brian: is it just Ganesh that puts the constant on the right of the > conditional, or does Skia do everything Yoda-style, too? The style guide (originally written by me so take it with a grain of salt) says Yoda-style. I think newer Skia code tries to follow that rule but I know I've seen exceptions.
Sign in to reply to this message.
https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.cpp File tools/render_pictures_main.cpp (right): https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... tools/render_pictures_main.cpp:42: SkFILEStream& inputStream) { On 2012/06/07 19:44:53, TomH wrote: > Skia style is to pass a pointer to non-const stuff like inputStream, so that > it's clearer in the calling context that you're passing something that may > change. > (And yeah, using char* instead of a string class makes this slightly > inconsistent...) Should I be wrapping the strings from the command line into SkStrings?
Sign in to reply to this message.
On 2012/06/07 21:00:57, keyar wrote: > https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.cpp > File tools/render_pictures_main.cpp (right): > > https://codereview.appspot.com/6300056/diff/8001/tools/render_pictures_main.c... > tools/render_pictures_main.cpp:42: SkFILEStream& inputStream) { > On 2012/06/07 19:44:53, TomH wrote: > > Skia style is to pass a pointer to non-const stuff like inputStream, so that > > it's clearer in the calling context that you're passing something that may > > change. > > (And yeah, using char* instead of a string class makes this slightly > > inconsistent...) > > Should I be wrapping the strings from the command line into SkStrings? I don't think it is necessary.
Sign in to reply to this message.
Added the python glue script.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
What are the future feature plans for this tool?
Sign in to reply to this message.
On 2012/06/08 18:19:36, reed1 wrote: > What are the future feature plans for this tool? This tool will be just to do fidelity tests. Future plans would be to add actual tests and maybe some nice minor features (such as being able to specify where skdiff or render_pictures is as you can do in compare_baselines.py). Since benchmark related tests are wanted, I think that should be a different tool or rolled into bench (I will look into that).
Sign in to reply to this message.
Got it. Here are some minor feature requests: (not married to the names) 1. --device-type (to get GPU images, or PDFs, or XPSs, or other picures) 2. --match ... (to restrict in some way which files are read) 3. --file ... (to specify an arbitrary picture file, instead of a directory) more exotic; 4. --tile (to draw into tiles, which we then stitch together to write the final png) 5. --scale (or --scale-to-fit ...) 6. --no-lcd-text (to disable lcd text and just use aa-text instead)
Sign in to reply to this message.
On 2012/06/08 19:03:13, reed1 wrote: > Got it. Here are some minor feature requests: (not married to the names) > > 1. --device-type (to get GPU images, or PDFs, or XPSs, or other picures) > 2. --match ... (to restrict in some way which files are read) > 3. --file ... (to specify an arbitrary picture file, instead of a directory) For specifying a particular file, perhaps we could have the inputDir (it would probably have to be renamed) argument take a comma separated list of files and folders to check? Perhaps something similar for the expectedDir argument as well. > > more exotic; > > 4. --tile (to draw into tiles, which we then stitch together to write the final > png) > 5. --scale (or --scale-to-fit ...) > 6. --no-lcd-text (to disable lcd text and just use aa-text instead) Do require some of these changes for landing? If not, could you commit this change and then create an issue for these features.
Sign in to reply to this message.
nothing from my list is required for landing. just wanted to (a) hear your reaction, and (b) ensure that none of those makes you want to change this CL for some reason.
Sign in to reply to this message.
On 2012/06/08 20:02:24, reed1 wrote: > nothing from my list is required for landing. just wanted to (a) hear your > reaction, and (b) ensure that none of those makes you want to change this CL for > some reason. I don't think any of those feature requests would require the CL to be changed for now. The argument parsing in render_pictures would have to be changed as soon as we add an option though.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
