Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184443003:
Add Gpu Tracing to Ganesh (Closed)
|
Created:
6 years, 9 months ago by egdaniel Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionAdd Gpu Tracing to Ganesh
BUG=skia:2316
Committed: http://code.google.com/p/skia/source/detail?r=13936
Patch Set 1 #Patch Set 2 : Partial version #
Total comments: 42
Patch Set 3 : Updates #Patch Set 4 : Added files #
Total comments: 9
Patch Set 5 : Generator change #Patch Set 6 : Android Fix #
Total comments: 1
Patch Set 7 : Use SkTLazy #
Total comments: 2
Patch Set 8 : Merge conflict fixes #Patch Set 9 : Merge fixes 2 #
Messages
Total messages: 34 (0 generated)
This is not a complete version. Just want to get some feed back at this point before connecting everything to the cpu markers
Overall I think this is looking nice. https://codereview.chromium.org/184443003/diff/40001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/184443003/diff/40001/include/gpu/GrContext.h#... include/gpu/GrContext.h:859: bool gpuTracingEnabled() const { return fGpuTracingEnabled; } Can we make it isGpuTracingEnabled()? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrContext.cpp#ne... src/gpu/GrContext.cpp:104: fGpuTracingEnabled = true; Let's start with false. https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrContext.cpp#ne... src/gpu/GrContext.cpp:1009: GR_CREATE_GPU_TRACE_MARKER("Test_marker_DRAW_ROUND_REC", target); .._RECT (T) https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.cpp File src/gpu/GrDrawTarget.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.cpp... src/gpu/GrDrawTarget.cpp:557: const char* currBaseCmd = ""; It took me a little bit to understand what this is used for. It might be clearer if it were named something like prevMarkerName since it is used to check whether the previous marker had the same name as the current one. https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:25: // class GrDrawTarget; ? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:450: bool gpuTracingEnabled() const; I wonder if we really need enable/disable control below GrContext level. To have it on GrDT might make for weird combinations (e.g. enabled on GrIODB but not the GrGpu object). https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:452: SkTDArray<GpuTraceMarker>* getActiveTraceMarkers() { return &fActiveTraceMarkers; } const &? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, Def. const & for markerArray. Also static functions begin with a capital letter. Can this be protected? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:889: virtual void onAddGpuTraceMarker() = 0; It's unclear from the header why we have all three of addTraceMarkerActiveSet() onAddGpuTraceMarker() addGpuTraceMarker(GpuTraceMarker* marker) (I haven't read the cpp yet but it should make sense from the .h) If on[Add|Remove]GpuTraceMarker() are notifiers that are called before or after some action in [add|remove]GpuTraceMarker we name them something like willAddGpuTraceMarker (if it is call before) or didAddGpuTraceMarker (if it is called after). https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:297: if (fCmds.back() & 0x08) { use enum value https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:527: if (fCmds[c] & 0x08) { use enum, or perhaps better add a function CmdHasTraceMarker(fCmds[c]) or something like that. https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:535: switch (stripTraceBit(fCmds[c])) { this-> but really the functions should probably be static https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:584: if (fCmds[c] & 0x08) { enum/function https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.cpp:869: addToCmdBuffer(kSetClip_Cmd); this-> here and below https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:254: return cmd | 0x08; Can we put these in an enum? e.g. enum { kTraceCmdBit = 0x80, kCmdMask = 0x7f, }; https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:258: return cmd & 0x07; 7f? https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:315: fGpuTracingSupport = true;//ctxInfo.hasExtension("GL_EXT_debug_marker"); :) https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLInterface... File src/gpu/gl/GrGLInterface.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLInterface... src/gpu/gl/GrGLInterface.cpp:478: #if 1 Can just remove the #if now that Chromium adds it (assuming that landed). https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGpuGL.cpp File src/gpu/gl/GrGpuGL.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGpuGL.cpp#n... src/gpu/gl/GrGpuGL.cpp:2671: printf("\n%s\n", markerString.c_str()); I assume this is just leftover debugging stuff
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrContext.cpp#ne... src/gpu/GrContext.cpp:104: fGpuTracingEnabled = true; That's the plan. Just set that way right now for ease of testing. On 2014/03/17 17:50:38, bsalomon wrote: > Let's start with false. https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.cpp File src/gpu/GrDrawTarget.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.cpp... src/gpu/GrDrawTarget.cpp:557: const char* currBaseCmd = ""; Agree. On 2014/03/17 17:50:38, bsalomon wrote: > It took me a little bit to understand what this is used for. It might be clearer > if it were named something like prevMarkerName since it is used to check whether > the previous marker had the same name as the current one. https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:25: // class GrDrawTarget; Woops. Left over from some old test iteration. Removed. On 2014/03/17 17:50:38, bsalomon wrote: > ? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:450: bool gpuTracingEnabled() const; This function is for controlling whether or not it is enabled, but just to check if it is. Regardless, this is only used in the macro to make a trace event, so that will get bubbled up to GrContext in the next iteration anyways. On 2014/03/17 17:50:38, bsalomon wrote: > I wonder if we really need enable/disable control below GrContext level. To have > it on GrDT might make for weird combinations (e.g. enabled on GrIODB but not the > GrGpu object). https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:452: SkTDArray<GpuTraceMarker>* getActiveTraceMarkers() { return &fActiveTraceMarkers; } in gl/GrGpuGl.cpp I call getActiveTraceMarkers(), and then send them to getTraceString which is not const (see comment below). On 2014/03/17 17:50:38, bsalomon wrote: > const &? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, markerArray ends up getting sorted in the function so it won't be const. Maybe function could be changed so that it assumes the markers are sorted and thus just extracts a string. Sorting would take place in some other call. On 2014/03/17 17:50:38, bsalomon wrote: > Def. const & for markerArray. Also static functions begin with a capital letter. > > Can this be protected? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:889: virtual void onAddGpuTraceMarker() = 0; So the basic structure here is: addGpuTraceMarker is the public interface to create one. Then the add/removeTraceMarkerActiveSet one performed all actions universal to all all DrawTargets. At one point these had a few commands for add/remove but at this point there is only a push() in addTMActiveSet() and a find() & remove(index) in removeTMActiveSet(), so we could just put these directly into add/removeGpuTraceMarker(). the onAddGpuTraceMarkers() call is used to perform code related to the specific DrawTarget. For example, GrIODB does nothing here, but GrGpuGL does have specilized code (like making the gl calls). Basically the code looks like: addGpuTraceMarker(marker) { addTraceMarkerActiveSet(marker); //done for all targets onAddGpuTracemarker(); // calls target specific code } Given this is there a standard way to name these things? On 2014/03/17 17:50:38, bsalomon wrote: > It's unclear from the header why we have all three of > > addTraceMarkerActiveSet() > onAddGpuTraceMarker() > addGpuTraceMarker(GpuTraceMarker* marker) > > (I haven't read the cpp yet but it should make sense from the .h) > > If on[Add|Remove]GpuTraceMarker() are notifiers that are called before or after > some action in [add|remove]GpuTraceMarker we name them something like > willAddGpuTraceMarker (if it is call before) or didAddGpuTraceMarker (if it is > called after). https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:254: return cmd | 0x08; yes! On 2014/03/17 17:50:38, bsalomon wrote: > Can we put these in an enum? > > e.g. > > enum { > kTraceCmdBit = 0x80, > kCmdMask = 0x7f, > }; https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:258: return cmd & 0x07; Currently the commands we use only go to 7 so I just used the next available bit, but I guess if we ever expand the set of commands its better to just use the far left bit. On 2014/03/17 17:50:38, bsalomon wrote: > 7f? https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLCaps.cpp#... src/gpu/gl/GrGLCaps.cpp:315: fGpuTracingSupport = true;//ctxInfo.hasExtension("GL_EXT_debug_marker"); another part of my debugging... On 2014/03/17 17:50:38, bsalomon wrote: > :) https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLInterface... File src/gpu/gl/GrGLInterface.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGLInterface... src/gpu/gl/GrGLInterface.cpp:478: #if 1 yup it landed On 2014/03/17 17:50:38, bsalomon wrote: > Can just remove the #if now that Chromium adds it (assuming that landed). https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGpuGL.cpp File src/gpu/gl/GrGpuGL.cpp (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/gl/GrGpuGL.cpp#n... src/gpu/gl/GrGpuGL.cpp:2671: printf("\n%s\n", markerString.c_str()); Yeah again a bunch of debug stuff throughout code which obviously will be removed once everything all nice. On 2014/03/17 17:50:38, bsalomon wrote: > I assume this is just leftover debugging stuff
Sorry for the delay! I started these comments a while back, got interrupted, and forgot to finish/publish them. https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:450: bool gpuTracingEnabled() const; On 2014/03/17 19:49:12, egdaniel wrote: > This function is for controlling whether or not it is enabled, but just to check > if it is. Regardless, this is only used in the macro to make a trace event, so > that will get bubbled up to GrContext in the next iteration anyways. > > On 2014/03/17 17:50:38, bsalomon wrote: > > I wonder if we really need enable/disable control below GrContext level. To > have > > it on GrDT might make for weird combinations (e.g. enabled on GrIODB but not > the > > GrGpu object). > sgtm https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, On 2014/03/17 19:49:12, egdaniel wrote: > markerArray ends up getting sorted in the function so it won't be const. Maybe > function could be changed so that it assumes the markers are sorted and thus > just extracts a string. Sorting would take place in some other call. Ok that makes sense. It'd be nice to hide this somehow, though, so that we don't have a public function on this class that returns a non-const ptr to the array. Maybe getTraceString should be a non-static member that sorts the array and converts it into a string? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:889: virtual void onAddGpuTraceMarker() = 0; On 2014/03/17 19:49:12, egdaniel wrote: > So the basic structure here is: addGpuTraceMarker is the public interface to > create one. Then the add/removeTraceMarkerActiveSet one performed all actions > universal to all all DrawTargets. At one point these had a few commands for > add/remove but at this point there is only a push() in addTMActiveSet() and a > find() & remove(index) in removeTMActiveSet(), so we could just put these > directly into add/removeGpuTraceMarker(). > > the onAddGpuTraceMarkers() call is used to perform code related to the specific > DrawTarget. For example, GrIODB does nothing here, but GrGpuGL does have > specilized code (like making the gl calls). > > Basically the code looks like: > addGpuTraceMarker(marker) { > addTraceMarkerActiveSet(marker); //done for all targets > onAddGpuTracemarker(); // calls target specific code > } > > Given this is there a standard way to name these things? Ok so addTraceMarkerActiveSet is just a helper/implementation function for addTraceMarker(). That seems fine. I'd name the virtual didAddGpuTraceMarker() since it is a notification to the subclass that the action was already performed. > On 2014/03/17 17:50:38, bsalomon wrote: > > It's unclear from the header why we have all three of > > > > addTraceMarkerActiveSet() > > onAddGpuTraceMarker() > > addGpuTraceMarker(GpuTraceMarker* marker) > > > > (I haven't read the cpp yet but it should make sense from the .h) > > > > If on[Add|Remove]GpuTraceMarker() are notifiers that are called before or > after > > some action in [add|remove]GpuTraceMarker we name them something like > > willAddGpuTraceMarker (if it is call before) or didAddGpuTraceMarker (if it is > > called after). > https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:258: return cmd & 0x07; On 2014/03/17 19:49:12, egdaniel wrote: > Currently the commands we use only go to 7 so I just used the next available > bit, but I guess if we ever expand the set of commands its better to just use > the far left bit. > On 2014/03/17 17:50:38, bsalomon wrote: > > 7f? > Yeah, I think it is safer to use 7f. If someone updates the bit enum (suggested in above comment) then they'll see the mask and update it. If someone adds a new Cmd they might forget to update it. addToCmdBuffer() should assert that the cmd param doesn't have any bits set not in 0x7f if it doesn't already.
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, So the function has already been moved to protected to hide it a little. But at this point what does changing it from static to non-static buy us? It doesn't need access to any specific data member, and it already "sorts the array and converts it into a string". So another option is to just not have it do any sorting and just do string conversion. In this case it will only merge strings/ID's together if they are contiguous in the array itself. Thus it is on the caller to sort the array themselves. We can also just claim the function will run but with undefined behavior if array is not sorted. On 2014/03/19 13:28:44, bsalomon wrote: > On 2014/03/17 19:49:12, egdaniel wrote: > > markerArray ends up getting sorted in the function so it won't be const. Maybe > > function could be changed so that it assumes the markers are sorted and thus > > just extracts a string. Sorting would take place in some other call. > > > Ok that makes sense. It'd be nice to hide this somehow, though, so that we don't > have a public function on this class that returns a non-const ptr to the array. > Maybe getTraceString should be a non-static member that sorts the array and > converts it into a string? https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrInOrderDrawBuf... src/gpu/GrInOrderDrawBuffer.h:258: return cmd & 0x07; Assert has been added in the newest version On 2014/03/19 13:28:44, bsalomon wrote: > On 2014/03/17 19:49:12, egdaniel wrote: > > Currently the commands we use only go to 7 so I just used the next available > > bit, but I guess if we ever expand the set of commands its better to just use > > the far left bit. > > On 2014/03/17 17:50:38, bsalomon wrote: > > > 7f? > > > > Yeah, I think it is safer to use 7f. If someone updates the bit enum (suggested > in above comment) then they'll see the mask and update it. If someone adds a new > Cmd they might forget to update it. > > addToCmdBuffer() should assert that the cmd param doesn't have any bits set not > in 0x7f if it doesn't already. >
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, On 2014/03/19 14:27:03, egdaniel wrote: > So the function has already been moved to protected to hide it a little. But at > this point what does changing it from static to non-static buy us? It doesn't > need access to any specific data member, and it already "sorts the array and > converts it into a string". > > So another option is to just not have it do any sorting and just do string > conversion. In this case it will only merge strings/ID's together if they are > contiguous in the array itself. Thus it is on the caller to sort the array > themselves. We can also just claim the function will run but with undefined > behavior if array is not sorted. > > On 2014/03/19 13:28:44, bsalomon wrote: > > On 2014/03/17 19:49:12, egdaniel wrote: > > > markerArray ends up getting sorted in the function so it won't be const. > Maybe > > > function could be changed so that it assumes the markers are sorted and thus > > > just extracts a string. Sorting would take place in some other call. > > > > > > Ok that makes sense. It'd be nice to hide this somehow, though, so that we > don't > > have a public function on this class that returns a non-const ptr to the > array. > > Maybe getTraceString should be a non-static member that sorts the array and > > converts it into a string? > It just seems to me like sorting-and-stringifying go together logically and by separating them we have to have this weird accessor that allows a caller to do arbitrary manipulation to the array. Really the only manipulation a caller wants to do is sort in order to call getTraceString(). Sorting is OK since the markers are effectively a set and unordered. The fact that the string produced by getTraceString() depends on the order of the array feels like implementation details bleeding out into the interface. That it is an array, that it is ok to reorder the array, and that getting the desired string requires sorting the array all seem like things that should be hidden in as few related functions as possible. I actually kind of wonder if exposing the trace markers as an array rather than as an iterator is a mistake... if we ever decide to store them differently then all callers that walk the array have to be changed. I guess if getActiveTraceMarkers() is protected then all the callers are in the GrDT family and that's not so bad.
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, Okay so here is my proposal (GpuTM is our trace marker class): 1) const GpuTM* activeTraceMarkersBegin() const GpuTM* activeTraceMarkersEnd() // thus iterating will be done by just incrementing the pointer 2) SkString getTraceStringFromActiveMarkers() // would do sorting internally 3) SkString getTraceString(GpuTM* start, GpuTM* end) // will sort between start and end, and then return trace string On 2014/03/19 14:43:43, bsalomon wrote: > On 2014/03/19 14:27:03, egdaniel wrote: > > So the function has already been moved to protected to hide it a little. But > at > > this point what does changing it from static to non-static buy us? It doesn't > > need access to any specific data member, and it already "sorts the array and > > converts it into a string". > > > > So another option is to just not have it do any sorting and just do string > > conversion. In this case it will only merge strings/ID's together if they are > > contiguous in the array itself. Thus it is on the caller to sort the array > > themselves. We can also just claim the function will run but with undefined > > behavior if array is not sorted. > > > > On 2014/03/19 13:28:44, bsalomon wrote: > > > On 2014/03/17 19:49:12, egdaniel wrote: > > > > markerArray ends up getting sorted in the function so it won't be const. > > Maybe > > > > function could be changed so that it assumes the markers are sorted and > thus > > > > just extracts a string. Sorting would take place in some other call. > > > > > > > > > Ok that makes sense. It'd be nice to hide this somehow, though, so that we > > don't > > > have a public function on this class that returns a non-const ptr to the > > array. > > > Maybe getTraceString should be a non-static member that sorts the array and > > > converts it into a string? > > > > > It just seems to me like sorting-and-stringifying go together logically and by > separating them we have to have this weird accessor that allows a caller to do > arbitrary manipulation to the array. Really the only manipulation a caller wants > to do is sort in order to call getTraceString(). Sorting is OK since the > markers are effectively a set and unordered. The fact that the string produced > by getTraceString() depends on the order of the array feels like implementation > details bleeding out into the interface. That it is an array, that it is ok to > reorder the array, and that getting the desired string requires sorting the > array all seem like things that should be hidden in as few related functions as > possible. > > I actually kind of wonder if exposing the trace markers as an array rather than > as an iterator is a mistake... if we ever decide to store them differently then > all callers that walk the array have to be changed. I guess if > getActiveTraceMarkers() is protected then all the callers are in the GrDT family > and that's not so bad.
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, Okay so here is my proposal (GpuTM is our trace marker class): 1) const GpuTM* activeTraceMarkersBegin() const GpuTM* activeTraceMarkersEnd() // thus iterating will be done by just incrementing the pointer 2) SkString getTraceStringFromActiveMarkers() // would do sorting internally 3) SkString getTraceString(GpuTM* start, GpuTM* end) // will sort between start and end, and then return trace string On 2014/03/19 14:43:43, bsalomon wrote: > On 2014/03/19 14:27:03, egdaniel wrote: > > So the function has already been moved to protected to hide it a little. But > at > > this point what does changing it from static to non-static buy us? It doesn't > > need access to any specific data member, and it already "sorts the array and > > converts it into a string". > > > > So another option is to just not have it do any sorting and just do string > > conversion. In this case it will only merge strings/ID's together if they are > > contiguous in the array itself. Thus it is on the caller to sort the array > > themselves. We can also just claim the function will run but with undefined > > behavior if array is not sorted. > > > > On 2014/03/19 13:28:44, bsalomon wrote: > > > On 2014/03/17 19:49:12, egdaniel wrote: > > > > markerArray ends up getting sorted in the function so it won't be const. > > Maybe > > > > function could be changed so that it assumes the markers are sorted and > thus > > > > just extracts a string. Sorting would take place in some other call. > > > > > > > > > Ok that makes sense. It'd be nice to hide this somehow, though, so that we > > don't > > > have a public function on this class that returns a non-const ptr to the > > array. > > > Maybe getTraceString should be a non-static member that sorts the array and > > > converts it into a string? > > > > > It just seems to me like sorting-and-stringifying go together logically and by > separating them we have to have this weird accessor that allows a caller to do > arbitrary manipulation to the array. Really the only manipulation a caller wants > to do is sort in order to call getTraceString(). Sorting is OK since the > markers are effectively a set and unordered. The fact that the string produced > by getTraceString() depends on the order of the array feels like implementation > details bleeding out into the interface. That it is an array, that it is ok to > reorder the array, and that getting the desired string requires sorting the > array all seem like things that should be hidden in as few related functions as > possible. > > I actually kind of wonder if exposing the trace markers as an array rather than > as an iterator is a mistake... if we ever decide to store them differently then > all callers that walk the array have to be changed. I guess if > getActiveTraceMarkers() is protected then all the callers are in the GrDT family > and that's not so bad.
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, On 2014/03/19 15:16:30, egdaniel wrote: > Okay so here is my proposal (GpuTM is our trace marker class): > > 1) const GpuTM* activeTraceMarkersBegin() > const GpuTM* activeTraceMarkersEnd() > // thus iterating will be done by just incrementing the pointer > > 2) SkString getTraceStringFromActiveMarkers() // would do sorting internally How about getActiveMarkersTraceString()? > > 3) SkString getTraceString(GpuTM* start, GpuTM* end) > // will sort between start and end, and then return trace string > Oh, I didn't realize you needed this. Why do you need a string of a subset? > On 2014/03/19 14:43:43, bsalomon wrote: > > On 2014/03/19 14:27:03, egdaniel wrote: > > > So the function has already been moved to protected to hide it a little. But > > at > > > this point what does changing it from static to non-static buy us? It > doesn't > > > need access to any specific data member, and it already "sorts the array and > > > converts it into a string". > > > > > > So another option is to just not have it do any sorting and just do string > > > conversion. In this case it will only merge strings/ID's together if they > are > > > contiguous in the array itself. Thus it is on the caller to sort the array > > > themselves. We can also just claim the function will run but with undefined > > > behavior if array is not sorted. > > > > > > On 2014/03/19 13:28:44, bsalomon wrote: > > > > On 2014/03/17 19:49:12, egdaniel wrote: > > > > > markerArray ends up getting sorted in the function so it won't be const. > > > Maybe > > > > > function could be changed so that it assumes the markers are sorted and > > thus > > > > > just extracts a string. Sorting would take place in some other call. > > > > > > > > > > > > Ok that makes sense. It'd be nice to hide this somehow, though, so that we > > > don't > > > > have a public function on this class that returns a non-const ptr to the > > > array. > > > > Maybe getTraceString should be a non-static member that sorts the array > and > > > > converts it into a string? > > > > > > > > > It just seems to me like sorting-and-stringifying go together logically and by > > separating them we have to have this weird accessor that allows a caller to do > > arbitrary manipulation to the array. Really the only manipulation a caller > wants > > to do is sort in order to call getTraceString(). Sorting is OK since the > > markers are effectively a set and unordered. The fact that the string produced > > by getTraceString() depends on the order of the array feels like > implementation > > details bleeding out into the interface. That it is an array, that it is ok to > > reorder the array, and that getting the desired string requires sorting the > > array all seem like things that should be hidden in as few related functions > as > > possible. > > > > I actually kind of wonder if exposing the trace markers as an array rather > than > > as an iterator is a mistake... if we ever decide to store them differently > then > > all callers that walk the array have to be changed. I guess if > > getActiveTraceMarkers() is protected then all the callers are in the GrDT > family > > and that's not so bad. >
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, So internally I have a single array that stores the markers for each command (different from the active set). A second array stores the marker count for each command. So for a given command I am only interested in a subset of the entire array. Checkout flush() in GrDrawTarget.h for a better understanding of how I use it. On 2014/03/19 15:27:01, bsalomon wrote: > On 2014/03/19 15:16:30, egdaniel wrote: > > Okay so here is my proposal (GpuTM is our trace marker class): > > > > 1) const GpuTM* activeTraceMarkersBegin() > > const GpuTM* activeTraceMarkersEnd() > > // thus iterating will be done by just incrementing the pointer > > > > 2) SkString getTraceStringFromActiveMarkers() // would do sorting internally > > How about getActiveMarkersTraceString()? > > > > > 3) SkString getTraceString(GpuTM* start, GpuTM* end) > > // will sort between start and end, and then return trace string > > > > Oh, I didn't realize you needed this. Why do you need a string of a subset? > > > > On 2014/03/19 14:43:43, bsalomon wrote: > > > On 2014/03/19 14:27:03, egdaniel wrote: > > > > So the function has already been moved to protected to hide it a little. > But > > > at > > > > this point what does changing it from static to non-static buy us? It > > doesn't > > > > need access to any specific data member, and it already "sorts the array > and > > > > converts it into a string". > > > > > > > > So another option is to just not have it do any sorting and just do string > > > > conversion. In this case it will only merge strings/ID's together if they > > are > > > > contiguous in the array itself. Thus it is on the caller to sort the array > > > > themselves. We can also just claim the function will run but with > undefined > > > > behavior if array is not sorted. > > > > > > > > On 2014/03/19 13:28:44, bsalomon wrote: > > > > > On 2014/03/17 19:49:12, egdaniel wrote: > > > > > > markerArray ends up getting sorted in the function so it won't be > const. > > > > Maybe > > > > > > function could be changed so that it assumes the markers are sorted > and > > > thus > > > > > > just extracts a string. Sorting would take place in some other call. > > > > > > > > > > > > > > > Ok that makes sense. It'd be nice to hide this somehow, though, so that > we > > > > don't > > > > > have a public function on this class that returns a non-const ptr to the > > > > array. > > > > > Maybe getTraceString should be a non-static member that sorts the array > > and > > > > > converts it into a string? > > > > > > > > > > > > > It just seems to me like sorting-and-stringifying go together logically and > by > > > separating them we have to have this weird accessor that allows a caller to > do > > > arbitrary manipulation to the array. Really the only manipulation a caller > > wants > > > to do is sort in order to call getTraceString(). Sorting is OK since the > > > markers are effectively a set and unordered. The fact that the string > produced > > > by getTraceString() depends on the order of the array feels like > > implementation > > > details bleeding out into the interface. That it is an array, that it is ok > to > > > reorder the array, and that getting the desired string requires sorting the > > > array all seem like things that should be hidden in as few related functions > > as > > > possible. > > > > > > I actually kind of wonder if exposing the trace markers as an array rather > > than > > > as an iterator is a mistake... if we ever decide to store them differently > > then > > > all callers that walk the array have to be changed. I guess if > > > getActiveTraceMarkers() is protected then all the callers are in the GrDT > > family > > > and that's not so bad. > > >
https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h File src/gpu/GrDrawTarget.h (right): https://codereview.chromium.org/184443003/diff/40001/src/gpu/GrDrawTarget.h#n... src/gpu/GrDrawTarget.h:454: static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray, On 2014/03/19 15:33:53, egdaniel wrote: > So internally I have a single array that stores the markers for each command > (different from the active set). A second array stores the marker count for each > command. So for a given command I am only interested in a subset of the entire > array. Checkout flush() in GrDrawTarget.h for a better understanding of how I > use it. > Got it. Do you think you get a lot of value out of a combined array? I'm wondering if what really this is asking for is class GrTraceMarkerSet that has add, remove, toString, and an iterator.
First possible good version. Should we leave in the current new Trace marker calls I have in GrContext?
Added new files
looking good https://codereview.chromium.org/184443003/diff/100001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/184443003/diff/100001/include/gpu/GrContext.h... include/gpu/GrContext.h:864: bool isGpuTracingEnabled() const { return fGpuTracingEnabled; } Why don't we just make the caller of this do ->getContext()->isGpuTracingEnabled() ? It feels a little weird to have this call on GrDrawTarget when really all GrDrawTargets under the same context have the same answer. Moreover, we still insert the marker if add is called, regardless of this value. https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrContext.cpp#n... src/gpu/GrContext.cpp:961: GR_CREATE_TRACE_MARKER("GrContext::drawVertices", target); Make sure you've groked the discussion on the other tracing issue (removing the old macros) about how these should be named. https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrInOrderDrawBu... File src/gpu/GrInOrderDrawBuffer.cpp (right): https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrInOrderDrawBu... src/gpu/GrInOrderDrawBuffer.cpp:121: namespace { why the namespace? https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrInOrderDrawBu... File src/gpu/GrInOrderDrawBuffer.h (right): https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrInOrderDrawBu... src/gpu/GrInOrderDrawBuffer.h:171: virtual void didAddGpuTraceMarker() SK_OVERRIDE {}; don't need ;s https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.cpp File src/gpu/GrTracing.cpp (right): https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.cpp#n... src/gpu/GrTracing.cpp:38: explicit Iter(const GrTraceMarkerSet* markers, int index) don't need explicit with 2 args, right? https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.h File src/gpu/GrTracing.h (right): https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:70: GrGpuTraceMarkerGenerator(const char* marker_str, int* marker_counter, GrDrawTarget* target); I think these should be inlinable. Otherwise we add a pair of function calls for every marker whether tracing is enabled or not. https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:78: ////////// macros to place around the internal draw calls ////////////////// Can we use the /** style comments? * */ https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:83: #define GR_CREATE_TRACE_MARKER(marker_str, target) \ If we want to do tracing in SkGpuDevice.cpp we'll need a version of this that operates on GrContext. But it seems OK to keep it at GrContext and below for now. https://codereview.chromium.org/184443003/diff/100001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:85: static int marker_counter = 0; \ I think these statics need some mangling to make sure they're unique if two are inserted in the same scope.
I changed up the marker generator in this version to do as minimal amount of work as possible, when tracing is not enabled.
I changed up the marker generator in this version to do as minimal amount of work as possible, when tracing is not enabled.
https://codereview.chromium.org/184443003/diff/140001/src/gpu/GrTracing.h File src/gpu/GrTracing.h (right): https://codereview.chromium.org/184443003/diff/140001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:44: struct Data { Check out SkTLazy.
lgtm
Please resolve before committing. https://codereview.chromium.org/184443003/diff/160001/src/gpu/GrTracing.h File src/gpu/GrTracing.h (right): https://codereview.chromium.org/184443003/diff/160001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:42: * cpu and gpu (if gpu tracing enabled) for the current scope. Just to check: does this default to always-on for CPU tracing? If so, performance on Android will be unacceptable; always-on TRACE_EVENT()s should be low-frequency.
https://codereview.chromium.org/184443003/diff/160001/src/gpu/GrTracing.h File src/gpu/GrTracing.h (right): https://codereview.chromium.org/184443003/diff/160001/src/gpu/GrTracing.h#new... src/gpu/GrTracing.h:42: * cpu and gpu (if gpu tracing enabled) for the current scope. So the cpu Trace events are set using TRACE_EVENT1 call. From my understanding these trace events are controlled by their category_group_enabled flags, which by default "skia.gpu" should be disabled as implemented below. On 2014/03/24 16:31:43, tomhudson wrote: > Just to check: does this default to always-on for CPU tracing? If so, > performance on Android will be unacceptable; always-on TRACE_EVENT()s should be > low-frequency.
> So the cpu Trace events are set using TRACE_EVENT1 call. From my understanding > these trace events are controlled by their category_group_enabled flags, which > by default "skia.gpu" should be disabled as implemented below. OK, sure. LGTM.
The CQ bit was checked by egdaniel@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/egdaniel@google.com/184443003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/gpu/GrContext.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file src/gpu/GrContext.cpp
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 102 (offset 1 line).
Hunk #3 succeeded at 772 (offset 1 line).
Hunk #4 succeeded at 894 (offset 1 line).
Hunk #5 succeeded at 951 (offset 1 line).
Hunk #6 succeeded at 1006 (offset 1 line).
Hunk #7 succeeded at 1028 (offset 1 line).
Hunk #8 succeeded at 1111 (offset 1 line).
Hunk #9 succeeded at 1148 (offset 1 line).
1 out of 9 hunks FAILED -- saving rejects to file src/gpu/GrContext.cpp.rej
Patch: src/gpu/GrContext.cpp
Index: src/gpu/GrContext.cpp
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index
c016d062abe1254d52da583ab7a301d38262d548..08d1b2f239a8a629f85515003d67718f5fc0d146
100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -25,6 +25,7 @@
#include "GrSoftwarePathRenderer.h"
#include "GrStencilBuffer.h"
#include "GrTextStrike.h"
+#include "GrTracing.h"
#include "SkRTConf.h"
#include "SkRRect.h"
#include "SkStrokeRec.h"
@@ -101,6 +102,7 @@ GrContext::GrContext() {
fOvalRenderer = NULL;
fViewMatrix.reset();
fMaxTextureSizeOverride = 1 << 20;
+ fGpuTracingEnabled = false;
}
bool GrContext::init(GrBackend backend, GrBackendContext backendContext) {
@@ -770,6 +772,8 @@ void GrContext::drawRect(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawRect", target);
+
SkScalar width = stroke == NULL ? -1 : stroke->getWidth();
SkMatrix combinedMatrix = target->drawState()->getViewMatrix();
if (NULL != matrix) {
@@ -890,6 +894,8 @@ void GrContext::drawRectToRect(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawRectToRect", target);
+
target->drawRect(dstRect, dstMatrix, &localRect, localMatrix);
}
@@ -945,6 +951,8 @@ void GrContext::drawVertices(const GrPaint& paint,
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawVertices", target);
+
GrDrawState* drawState = target->drawState();
int colorOffset = -1, texOffset = -1;
@@ -998,6 +1006,8 @@ void GrContext::drawRRect(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawRRect", target);
+
if (!fOvalRenderer->drawSimpleRRect(target, this, paint.isAntiAlias(),
rect, stroke)) {
SkPath path;
path.addRRect(rect);
@@ -1018,6 +1028,8 @@ void GrContext::drawOval(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawOval", target);
+
if (!fOvalRenderer->drawOval(target, this, paint.isAntiAlias(), oval,
stroke)) {
SkPath path;
path.addOval(oval);
@@ -1099,6 +1111,8 @@ void GrContext::drawPath(const GrPaint& paint, const
SkPath& path, const SkStrok
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
GrDrawState* drawState = target->drawState();
+ GR_CREATE_TRACE_MARKER("GrContext::drawPath", target);
+
bool useCoverageAA = paint.isAntiAlias() &&
!drawState->getRenderTarget()->isMultisampled();
if (useCoverageAA && stroke.getWidth() < 0 && !path.isConvex()) {
@@ -1134,6 +1148,9 @@ void GrContext::internalDrawPath(GrDrawTarget* target,
bool useAA, const SkPath&
const SkStrokeRec& origStroke) {
SkASSERT(!path.isEmpty());
+ GR_CREATE_TRACE_MARKER("GrContext::internalDrawPath", target);
+
+
// An Assumption here is that path renderer would use some form of tweaking
// the src color (either the input alpha or in the frag shader) to
implement
// aa. If we have some future driver-mojo path AA that can do the right
The CQ bit was checked by egdaniel@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/egdaniel@google.com/184443003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/gpu/GrContext.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file src/gpu/GrContext.cpp
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 101 (offset -1 lines).
Hunk #3 succeeded at 771 (offset -1 lines).
Hunk #4 succeeded at 893 (offset -1 lines).
Hunk #5 succeeded at 950 (offset -1 lines).
Hunk #6 succeeded at 1005 (offset -1 lines).
Hunk #7 succeeded at 1027 (offset -1 lines).
Hunk #8 succeeded at 1110 (offset -1 lines).
Hunk #9 succeeded at 1147 (offset -1 lines).
1 out of 9 hunks FAILED -- saving rejects to file src/gpu/GrContext.cpp.rej
Patch: src/gpu/GrContext.cpp
Index: src/gpu/GrContext.cpp
diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index
9a277b05917fcfc0516d1fa6f40add5f20dc1d6d..27d8dc679ec94629a22c36ee610a30d581d26915
100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -25,6 +25,7 @@
#include "GrSoftwarePathRenderer.h"
#include "GrStencilBuffer.h"
#include "GrTextStrike.h"
+#include "GrTracing.h"
#include "SkGr.h"
#include "SkRTConf.h"
#include "SkRRect.h"
@@ -102,6 +103,7 @@ GrContext::GrContext() {
fOvalRenderer = NULL;
fViewMatrix.reset();
fMaxTextureSizeOverride = 1 << 20;
+ fGpuTracingEnabled = false;
}
bool GrContext::init(GrBackend backend, GrBackendContext backendContext) {
@@ -771,6 +773,8 @@ void GrContext::drawRect(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawRect", target);
+
SkScalar width = stroke == NULL ? -1 : stroke->getWidth();
SkMatrix combinedMatrix = target->drawState()->getViewMatrix();
if (NULL != matrix) {
@@ -891,6 +895,8 @@ void GrContext::drawRectToRect(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawRectToRect", target);
+
target->drawRect(dstRect, dstMatrix, &localRect, localMatrix);
}
@@ -946,6 +952,8 @@ void GrContext::drawVertices(const GrPaint& paint,
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawVertices", target);
+
GrDrawState* drawState = target->drawState();
int colorOffset = -1, texOffset = -1;
@@ -999,6 +1007,8 @@ void GrContext::drawRRect(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawRRect", target);
+
if (!fOvalRenderer->drawSimpleRRect(target, this, paint.isAntiAlias(),
rect, stroke)) {
SkPath path;
path.addRRect(rect);
@@ -1019,6 +1029,8 @@ void GrContext::drawOval(const GrPaint& paint,
AutoCheckFlush acf(this);
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
+ GR_CREATE_TRACE_MARKER("GrContext::drawOval", target);
+
if (!fOvalRenderer->drawOval(target, this, paint.isAntiAlias(), oval,
stroke)) {
SkPath path;
path.addOval(oval);
@@ -1100,6 +1112,8 @@ void GrContext::drawPath(const GrPaint& paint, const
SkPath& path, const SkStrok
GrDrawTarget* target = this->prepareToDraw(&paint, BUFFERED_DRAW, &are,
&acf);
GrDrawState* drawState = target->drawState();
+ GR_CREATE_TRACE_MARKER("GrContext::drawPath", target);
+
bool useCoverageAA = paint.isAntiAlias() &&
!drawState->getRenderTarget()->isMultisampled();
if (useCoverageAA && stroke.getWidth() < 0 && !path.isConvex()) {
@@ -1135,6 +1149,9 @@ void GrContext::internalDrawPath(GrDrawTarget* target,
bool useAA, const SkPath&
const SkStrokeRec& origStroke) {
SkASSERT(!path.isEmpty());
+ GR_CREATE_TRACE_MARKER("GrContext::internalDrawPath", target);
+
+
// An Assumption here is that path renderer would use some form of tweaking
// the src color (either the input alpha or in the frag shader) to
implement
// aa. If we have some future driver-mojo path AA that can do the right
The CQ bit was checked by egdaniel@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/egdaniel@google.com/184443003/390002
Message was sent while issue was closed.
Change committed as 13936 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
