Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15967010:
Add skiaBenchmarking.getOps() (Closed)
|
Created:
7 years, 6 months ago by f(malita) Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, reed1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd skiaBenchmarking.getOps()
Given a JSON-encoded Picture, the new function returns an array of Skia draw commands + info.
Depends on https://codereview.chromium.org/16638014/.
Re-landing after Skia SkDebugCanvas static initializer fix (https://code.google.com/p/skia/source/detail?r=9723).
R=nduca@chromium.org, piman@chromium.org, senorblanco@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207676
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208695
Patch Set 1 #
Total comments: 10
Patch Set 2 : Removed compile time flag, added unittest. #Patch Set 3 : Leave Picture::Raster() alone for now. #Patch Set 4 : Remove stale comment & empty clip after picture.cc revert. #Patch Set 5 : Refactored unit test. #Patch Set 6 : Zap unneeded skia_benchmarking_extension.h include. #Patch Set 7 : Speculative "Win" bot compilation fix. #
Total comments: 7
Patch Set 8 : Helper test function instead of macro. #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... File content/renderer/skia_benchmarking_extension.cc (right): https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... content/renderer/skia_benchmarking_extension.cc:65: " @returns [{ 'cmd': {String}, 'info': [String, ...] }, ...]" define in the docs that undefined is returned when the picture is an old version and wont load... https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... content/renderer/skia_benchmarking_extension.cc:175: SkTDArray<SkString*>* info = canvas.getCommandInfo(i); what is in the info? what is mandatory and what is optional? We dont want a roll of skia to randomly break the frame viewer. Lets make a _unittest.cc for this file, and add some tests that record a picture, then get cmd info on it and asser tthat the things we expect to require are in this array. That way a skia deps roll will explode at the right time. https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... content/renderer/skia_benchmarking_extension.cc:176: if (info) { why would info not be returned? It seems odd that some will have info and some not.... we should clarify that too https://codereview.chromium.org/15967010/diff/1/skia/skia.gyp File skia/skia.gyp (right): https://codereview.chromium.org/15967010/diff/1/skia/skia.gyp#newcode11 skia/skia.gyp:11: # We have to nest variables inside variables so that they can be overridden confused, this wont be always on? If thats the case, we're doing something wrong... this system should be always-compiled in, not on-in-some-builds-but-not-others... https://codereview.chromium.org/15967010/diff/1/skia/skia.gyp#newcode568 skia/skia.gyp:568: 'SK_DEVELOPER', what implications does this have?
https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... File content/renderer/skia_benchmarking_extension.cc (right): https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... content/renderer/skia_benchmarking_extension.cc:65: " @returns [{ 'cmd': {String}, 'info': [String, ...] }, ...]" On 2013/06/11 23:52:13, nduca wrote: > define in the docs that undefined is returned when the picture is an old version > and wont load... Ok. https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... content/renderer/skia_benchmarking_extension.cc:175: SkTDArray<SkString*>* info = canvas.getCommandInfo(i); On 2013/06/11 23:52:13, nduca wrote: > what is in the info? what is mandatory and what is optional? We dont want a roll > of skia to randomly break the frame viewer. > > Lets make a _unittest.cc for this file, and add some tests that record a > picture, then get cmd info on it and asser tthat the things we expect to require > are in this array. That way a skia deps roll will explode at the right time. This is based on the current Skia debugger backend, where the format is loosely defined and we probably shouldn't use info for anything beyond displaying the strings (same as the Skia debugger does). The plan is to revisit the debugger backend classes, and clean-up all this (https://code.google.com/p/skia/issues/detail?id=1341). Ideally we'll end up with some form of serialization for draw command parameters, and proper v8 wrapper types. At that point we can start relying on info fields and capture the behavior in unit tests. It would make sense to wait for the Skia cleanup before adding this, but pdr suggested that you guys are blocked on getOps and even a simple command list would help. https://codereview.chromium.org/15967010/diff/1/content/renderer/skia_benchma... content/renderer/skia_benchmarking_extension.cc:176: if (info) { On 2013/06/11 23:52:13, nduca wrote: > why would info not be returned? It seems odd that some will have info and some > not.... we should clarify that too Was following Skia use here, but you're right - it cannot be null. The list can be empty thought for commands with no args (restore, etc), but that is guarded below. https://codereview.chromium.org/15967010/diff/1/skia/skia.gyp File skia/skia.gyp (right): https://codereview.chromium.org/15967010/diff/1/skia/skia.gyp#newcode11 skia/skia.gyp:11: # We have to nest variables inside variables so that they can be overridden On 2013/06/11 23:52:13, nduca wrote: > confused, this wont be always on? If thats the case, we're doing something > wrong... this system should be always-compiled in, not > on-in-some-builds-but-not-others... Alright... was hesitant to go there, yet :) I don't expect the annotations framework to have a runtime impact, but I'm somewhat wary of SK_DEVELOPER below. https://codereview.chromium.org/15967010/diff/1/skia/skia.gyp#newcode568 skia/skia.gyp:568: 'SK_DEVELOPER', On 2013/06/11 23:52:13, nduca wrote: > what implications does this have? It mostly enables toString methods on some Skia types - for example, without it getCommandInfo() doesn't show paint info. But there's something more going on in SkPicturePlayback, I'll have to check with Mike what the intent is. So the impact is hopefully just a small code size increase. Possibly enough reason to keep this behind a compile flag?
We need about:tracing features to be always-on, so any compile time option is basically a non-starter. What makes a feature useful in about-tracing is that you can get the data out of any chrome build. If we make this a compile time option, then the number of people who can use the about:tracing debugger has dropped calamatously. So maybe you can pull the thread and figure out how to to either 1. move the stuff-we-need-for-debugger out of SK_DEVELOPER and into the default build 2. make sk_developer on always Code size increase is okay, i think as long as its not calamatous. If need be, I can shrink the size of about:tracing's javascript (which comes in at several hundred Kb) so it comes out as a wash for our TPMs. Point being, lets deal with that later, not now. :) To the required fields discussion, maybe you can land a basic unittest that just verifies that you got back some basic sane opcodes. That way when we do start depending on certain types of info coming back, we'll have the harness in place for adding more test coverage. Otherwise what will happen (and always happens) is we'll add a dependency on some field in a big huge hurry, and the argument will go something like "I dont have time to write that test, this is important, blahblah."
On 2013/06/12 19:19:09, nduca wrote: > We need about:tracing features to be always-on, so any compile time option is > basically a non-starter. What makes a feature useful in about-tracing is that > you can get the data out of any chrome build. If we make this a compile time > option, then the number of people who can use the about:tracing debugger has > dropped calamatously. So maybe you can pull the thread and figure out how to to > either > 1. move the stuff-we-need-for-debugger out of SK_DEVELOPER and into the default > build > 2. make sk_developer on always > > Code size increase is okay, i think as long as its not calamatous. If need be, I > can shrink the size of about:tracing's javascript (which comes in at several > hundred Kb) so it comes out as a wash for our TPMs. Point being, lets deal with > that later, not now. :) OK, will update. > To the required fields discussion, maybe you can land a basic unittest that just > verifies that you got back some basic sane opcodes. SGTM. Figuring how to mock a JS context right now :)
I think you can unit test a layer lower -- e.g. you dont have to test the extension... just test at the skia levelthat that tdict that comes back has the sane things. The actual extension code is not going to change much so it doesnt haveas much need to be tested
On 2013/06/13 21:32:21, nduca wrote: > I think you can unit test a layer lower -- e.g. you dont have to test the > extension... just test at the skia levelthat that tdict that comes back has the > sane things. The actual extension code is not going to change much so it doesnt > haveas much need to be tested We can certainly drop one layer down, but just to make sure we're on the same page: there are two things coming out of SkDebugCanvas at the moment - * an array of SkDrawCommand (getDrawCommandAt(int index)) - this is a well defined interface, but the only useful bits of information are the command type enum (DrawType) and its string representation (SkDrawCommand::GetCommandString(cmd_type)). Makes sense to unit-test. * for each draw command, an array of strings with additional info (SkTDArray<SkString*> via getCommandInfo(int index)) - this is poorly defined and a somewhat arbitrary representation of the command parameters (not even a dictionary). We can unit test some of these, with the caveat that their format will change soonish to something more flexible (where we could pass in a specialized serializer for example, and get back a JSON or v8::DictionaryValue encoding). But at this point we should probably not do anything with these strings beyond displaying.
Tried too early - the bots haven't picked up the Skia roll yet.
lgtm this is awesome
On 2013/06/14 18:24:09, nduca wrote:
> lgtm this is awesome
Thanks.
Any idea what the "Win" builder is smoking? SkTDArray is already used in another
content file and gets instantiated correctly on all other builders:
content.skia_benchmarking_extension.obj : error LNK2019: unresolved external
symbol "__declspec(dllimport) public: class SkString * & __thiscall
SkTDArray<class SkString *>::getAt(int)"
(__imp_?getAt@?$SkTDArray@PAVSkString@@@@QAEAAPAVSkString@@H@Z) referenced in
function "public: static void __cdecl `anonymous
namespace'::SkiaBenchmarkingWrapper::GetOps(class v8::FunctionCallbackInfo<class
v8::Value> const &)"
(?GetOps@SkiaBenchmarkingWrapper@?A0xef739262@@SAXABV?$FunctionCallbackInfo@VValue@v8@@@v8@@@Z)
There's naming clash on Mac too:
In file included from ../../content/renderer/skia_benchmarking_extension.cc:15:
In file included from
../../third_party/skia/src/utils/debugger/SkDebugCanvas.h:14:
../../third_party/skia/src/utils/debugger/SkDrawCommand.h:251:7: error:
redefinition of 'Comment'
class Comment : public SkDrawCommand {
^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/AIFF.h:192:8:
note: previous definition is here
struct Comment {
^
1 error generated.
I'm not familiar with Mac builds - is there some way to suppress automatic
framework headers inclusion per compilation unit? I'll fix this in Skia, but was
hoping to land it today :(
On 2013/06/14 20:06:49, Florin Malita wrote: > > Any idea what the "Win" builder is smoking? SkTDArray is already used in another > content file and gets instantiated correctly on all other builders: And now the Win build compiles after switching to using the index operator instead of getAt()... a mystery I'm happy to leave untouched :)
I'm terrible at these things. jamesr tends to be very smart about this if you want to ask him on irc.
The Skia Mac naming fix is in (https://codereview.chromium.org/17101005/) so we should be able to land this as soon as Skia rolls. Unfortunately, there seems to be a problem preventing Skia rolls atm :(
On 2013/06/19 13:43:01, Florin Malita wrote: > Unfortunately, there seems to be a problem preventing Skia rolls atm :( Skia rolled! Waiting for the bots/LKGR to catch up with the roll, then we can land this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmalita@chromium.org/15967010/31001
On 2013/06/20 18:17:24, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/fmalita%40chromium.org/15967010/31001 Whoops, need skia & content owner lgtms.
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
LGTM overall, 1 question, 1 nit. https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... File content/renderer/skia_benchmarking_extension.cc (right): https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... content/renderer/skia_benchmarking_extension.cc:163: scoped_refptr<cc::Picture> picture = ParsePictureArg(args[0]); Just to confirm, this extension is behind a flag, right? We're not exposing "parse an arbitrary bag of bytes and put it in a SkPicture" to the drive-by web? https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... File content/renderer/skia_benchmarking_extension_unittest.cc (right): https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... content/renderer/skia_benchmarking_extension_unittest.cc:26: } while (0) nit: Why the need to make this a macro? Can't it be a helper function?
https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... File content/renderer/skia_benchmarking_extension.cc (right): https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... content/renderer/skia_benchmarking_extension.cc:163: scoped_refptr<cc::Picture> picture = ParsePictureArg(args[0]); On 2013/06/20 18:30:52, piman wrote: > Just to confirm, this extension is behind a flag, right? We're not exposing > "parse an arbitrary bag of bytes and put it in a SkPicture" to the drive-by web? Yes, this is all behind --enable-skia-benchmarking, which also prints a scary warning. https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... File content/renderer/skia_benchmarking_extension_unittest.cc (right): https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... content/renderer/skia_benchmarking_extension_unittest.cc:26: } while (0) On 2013/06/20 18:30:52, piman wrote: > nit: Why the need to make this a macro? Can't it be a helper function? The only reason I went with a macro was to be able to pass a token-pasted parameter to EXPECT_TRUE, and get a more informative message in case of failure (containing the actual missing parameter name). Maybe it's not worth it, or maybe there's a better way?
LGTM https://codereview.chromium.org/15967010/diff/31001/skia/skia.gyp File skia/skia.gyp (right): https://codereview.chromium.org/15967010/diff/31001/skia/skia.gyp#newcode137 skia/skia.gyp:137: '../third_party/skia/src/utils/debugger/SkObjectParser.h', Note for posterity: we should probably create a utils.gypi in upstream skia at some point, and include it here.
https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... File content/renderer/skia_benchmarking_extension_unittest.cc (right): https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... content/renderer/skia_benchmarking_extension_unittest.cc:26: } while (0) On 2013/06/20 18:48:15, Florin Malita wrote: > On 2013/06/20 18:30:52, piman wrote: > > nit: Why the need to make this a macro? Can't it be a helper function? > > The only reason I went with a macro was to be able to pass a token-pasted > parameter to EXPECT_TRUE, and get a more informative message in case of failure > (containing the actual missing parameter name). Maybe it's not worth it, or > maybe there's a better way? gtest.h defines the AssertionResult class which looks like it would fit the role (there's a few examples there).
jumps on the lgtmtrain
https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... File content/renderer/skia_benchmarking_extension_unittest.cc (right): https://codereview.chromium.org/15967010/diff/31001/content/renderer/skia_ben... content/renderer/skia_benchmarking_extension_unittest.cc:26: } while (0) On 2013/06/20 19:35:03, piman wrote: > gtest.h defines the AssertionResult class which looks like it would fit the role > (there's a few examples there). That's perfect, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmalita@chromium.org/15967010/50003
Message was sent while issue was closed.
Committed patchset #8 manually as r207676 (presubmit successful).
Message was sent while issue was closed.
On 2013/06/21 02:58:54, Florin Malita wrote: > Committed patchset #8 manually as r207676 (presubmit successful). ... and it got rolled out because SkDebugCanvas introduced a static initializer :( ... from an <iostream> include ... which is unused </picard_meme>
Message was sent while issue was closed.
now you have to get that up on g+ as a memegen
Message was sent while issue was closed.
Skia-side fix is in: https://code.google.com/p/skia/source/detail?r=9723 Waiting for the roll to re-land this...
On 2013/06/22 16:09:18, Florin Malita wrote: > Skia-side fix is in: https://code.google.com/p/skia/source/detail?r=9723 > > Waiting for the roll to re-land this... Skia has rolleth: https://src.chromium.org/viewvc/chrome?view=rev&revision=208681 Should be safe to re-land now.
Message was sent while issue was closed.
Committed patchset #8 manually as r208695 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
