Issue 6457099: Added 1D texture atlas object. (Closed)
|
Created:
12 years, 3 months ago by rileya Modified:
12 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdded GrTextureStripAtlas object.
Committed: https://code.google.com/p/skia/source/detail?r=5070
Patch Set 1 #
Total comments: 1
Patch Set 2 : Stray tabs->spaces #
Total comments: 10
Patch Set 3 : #
Total comments: 17
Patch Set 4 : Add this-> to member function calls. #
Total comments: 20
Patch Set 5 : #
Total comments: 1
Patch Set 6 : Fix one last nit #Patch Set 7 : Use GrCacheData. #
MessagesTotal messages: 9
It's not actually hooked up to anything yet; I have it working with gradients, but I'll do a separate CL for that. Since it adds two files I assume this requires some gyp/DEPs magic. http://codereview.appspot.com/6457099/diff/1/src/gpu/effects/Gr1DTextureAtlas... File src/gpu/effects/Gr1DTextureAtlas.cpp (right): http://codereview.appspot.com/6457099/diff/1/src/gpu/effects/Gr1DTextureAtlas... src/gpu/effects/Gr1DTextureAtlas.cpp:16: const uint64_t Gr1DTextureAtlas::gCacheID = 0xfffffffffffffff0; Everything's in place for the texture domain stuff now, right?
Sign in to reply to this message.
Overall this looks nice. It might be good to have a validate() function (as some other classes in Skia/Gr do) that checks (via asserts) invariants of the data class (e.g. the linked list has the right number of entries) that gets called at the beginning and and of interesting member functions. It makes easier for someone else to later make mods to this class and know they haven't messed something up. One thing it could check is that the all of the unlocked entries are less recently used than the locked entries. If we start introducing out-of-order drawing it would help us remember to update the logic here that finds a row (maybe it would no longer only check the LRU entry). http://codereview.appspot.com/6457099/diff/4001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): http://codereview.appspot.com/6457099/diff/4001/include/gpu/GrContext.h#newco... include/gpu/GrContext.h:820: friend class Gr1DTextureAtlas; Can we move this up to the to GrAtlas friend above so it can share the comment? http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... File src/gpu/effects/Gr1DTextureAtlas.cpp (right): http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.cpp:104: GrAssert(NULL != row); Maybe we should return an illegal value to the to caller (-1) and in the gradient case it could have the current behavior (uses an individual texture). http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... File src/gpu/effects/Gr1DTextureAtlas.h (right): http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.h:24: class Gr1DTextureAtlas { GrTextureStripAtlas? That's not great either, but the current name suggests that each entry is always 1 row. GrFixedSizeTextureAtlas? http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.h:64: GrScalar getScaleFactor() { return static_cast<GrScalar>(fRowHeight) / fHeight; } I think you want SkIntToScalar here rather than static cast. http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.h:157: // Array of AtlasRows which store the state of all our rows Should we say ordered by position in the texture?
Sign in to reply to this message.
I added a validate() function that gets called before and after most of the nontrivial public functions in debug mode. http://codereview.appspot.com/6457099/diff/4001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): http://codereview.appspot.com/6457099/diff/4001/include/gpu/GrContext.h#newco... include/gpu/GrContext.h:820: friend class Gr1DTextureAtlas; On 2012/08/10 14:48:45, bsalomon wrote: > Can we move this up to the to GrAtlas friend above so it can share the comment? Done. http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... File src/gpu/effects/Gr1DTextureAtlas.cpp (right): http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.cpp:104: GrAssert(NULL != row); On 2012/08/10 14:48:45, bsalomon wrote: > Maybe we should return an illegal value to the to caller (-1) and in the > gradient case it could have the current behavior (uses an individual texture). Done. http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... File src/gpu/effects/Gr1DTextureAtlas.h (right): http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.h:24: class Gr1DTextureAtlas { On 2012/08/10 14:48:45, bsalomon wrote: > GrTextureStripAtlas? That's not great either, but the current name suggests > that each entry is always 1 row. > GrTextureStripAtlas seems to work okay. http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.h:64: GrScalar getScaleFactor() { return static_cast<GrScalar>(fRowHeight) / fHeight; } On 2012/08/10 14:48:45, bsalomon wrote: > I think you want SkIntToScalar here rather than static cast. Fixed (also changed this math such that yCoord + this = the center of the last pixel in the row, rather than the next pixel outside the row). http://codereview.appspot.com/6457099/diff/4001/src/gpu/effects/Gr1DTextureAt... src/gpu/effects/Gr1DTextureAtlas.h:157: // Array of AtlasRows which store the state of all our rows On 2012/08/10 14:48:45, bsalomon wrote: > Should we say ordered by position in the texture? Added a note about this.
Sign in to reply to this message.
LGTM. I'd say let's hold off landing it because of the texture key stuff that is in progress but since it isn't being used I think it's OK. One style nit: member function calls from within the class should be this->memberFunc().
Sign in to reply to this message.
Pardon my pickiness... http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... File src/gpu/effects/GrTextureStripAtlas.cpp (right): http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:27: class AtlasEntry; I wanted to quibble with the name, but I guess it works. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:42: AtlasEntry* entry = gAtlasCache.find(lookup.fKey); Why do we create a lookup & set its key, then pass in the key, rather than just creating a key and setting it directly? http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:63: , fRows(new AtlasRow[fNumRows]) Can we use SkNEW_ARRAY? http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:72: delete[] fRows; ...in which case we'd have to use SkDELETE_ARRAY http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:93: // pull out of the LRU if it was unlocked Not a very useful comment? http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:113: return -1; Is this "trying again"? Or failing? http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:116: // If the row we've picked had data in it before, pull it out of the table The following code block isn't very clear, even with this comment. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:146: *yCoord = (static_cast<GrScalar>(rowNumber) + GR_ScalarHalf) / (fHeight / fRowHeight); getScaleFactor() uses rowHeight - 1 / height; is that multiply (?) really going to be equivalent to this divide? http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:214: // push row onto the list Not a useful comment? http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... File src/gpu/effects/GrTextureStripAtlas.h (right): http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:51: * @param yCoord The y offset into the texture to use when doing a lookup (may be NULL) texture: bitmap? What is the meaning of a NULL value here? The comment to getScaleFactor suggests that this is an out value; how is it different from the int you return? http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:52: * @param key A key that uniquely identifies this bitmap data (default: bitmap genID) Do we ever need to use a non-default key? Here again I want to bikeshed the name, but I guess it works. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:65: * within a row. This + a yCoord returned by lockRow points to the center of the last I'd expect a scale to be *, not +? What does the "last pixel in a row" mean? I'm afraid that although I think I understand how the atlas is laid out, I don't understand the comments on your API. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:68: GrScalar getScaleFactor() { return SkIntToScalar(fRowHeight - 1) / fHeight; } nit: const http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:103: * Helper that unlocks the texture nit: is this comment really telling me anything? http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:118: * Pull a row out of the LRU list (while a row is locked, we pull it out of the list entirely) The unparenthetical is obvious; the parenthetical is opaque. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:123: * Searches the key table for a key and returns the index (or ~(insertion index) if not found) What is insertion index? http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:129: * used unlocked row. Returns NULL otherwise. Nit: runon sentence could be tighter. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:147: // Used to uniquely identify our textures in the texture cache Single static value uniquely identifying *multiple* textures? http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:149: // This is separate from gCacheID and is a uint32_t so we can use sk_atomic_inc to increment It's obvious that this is seperate from gCacheID, and it's nice to know why it's the size it is, but it isn't obvious what it's being used for or why the fact that it's separate from gCacheID is worth mentioning? http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:156: GrContext* fContext; We embed (approximately; see next comment) all the members of a desc - should we just embed one instead? I could be convinced either way... http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:161: const int fWidth; You use different types here than you use in a desc?
Sign in to reply to this message.
Alright, I think I addressed everything. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... File src/gpu/effects/GrTextureStripAtlas.cpp (right): http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:42: AtlasEntry* entry = gAtlasCache.find(lookup.fKey); On 2012/08/10 21:50:17, TomH wrote: > Why do we create a lookup & set its key, then pass in the key, rather than just > creating a key and setting it directly? I'd pasted this from another use of GrTHashTable and had forgotten to tidy this up, fixed. It also never ended up deleting the atlases/entries, so I did sort of a hack to ensure it happens on exit (the hash table can only take pointers, so I wrapped the entry instances in a struct with a destructor that deletes them at exit). What really needs to happen, is for there to be some way of knowing when an Atlas's GrContext gets destroyed so we can clean it up then (otherwise if a long-lived application made a bunch of GrContexts and atlases over time, they'd leak...), the textures are unlocked properly as-is, but the row data and such would remain until exit. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:63: , fRows(new AtlasRow[fNumRows]) On 2012/08/10 21:50:17, TomH wrote: > Can we use SkNEW_ARRAY? Done. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:72: delete[] fRows; On 2012/08/10 21:50:17, TomH wrote: > ...in which case we'd have to use SkDELETE_ARRAY And done. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:93: // pull out of the LRU if it was unlocked On 2012/08/10 21:50:17, TomH wrote: > Not a very useful comment? Deleted. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:113: return -1; On 2012/08/10 21:50:17, TomH wrote: > Is this "trying again"? Or failing? Whoops! Good catch, this was an assert that was supposed to turn into a conditional with return -1, but I left out the conditional, fixed. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:116: // If the row we've picked had data in it before, pull it out of the table On 2012/08/10 21:50:17, TomH wrote: > The following code block isn't very clear, even with this comment. I tried to make it a bit clearer. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:146: *yCoord = (static_cast<GrScalar>(rowNumber) + GR_ScalarHalf) / (fHeight / fRowHeight); On 2012/08/10 21:50:17, TomH wrote: > getScaleFactor() uses rowHeight - 1 / height; is that multiply (?) really going > to be equivalent to this divide? This was supposed to be the y-offset in the atlas to the start of our row in [0,1] for passage to a shader, so you'd multiply whatever your original texture y-coordinate was by the scale factor and add the offset. I removed this as an output param, and moved it a public function that takes a row and gives you the appropriate y-offset (also named it better and gave it a clearer explanation, see the header). I'd also been offsetting by half a pixel here to simplify the 1d texture case (so the y-coordinate points at the center of the row, rather than the the top edge of it, so you could pass it straight in as a uniform), but that made things a lot less clear in any other case, so I got rid of that. http://codereview.appspot.com/6457099/diff/7002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:214: // push row onto the list On 2012/08/10 21:50:17, TomH wrote: > Not a useful comment? Gone. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... File src/gpu/effects/GrTextureStripAtlas.h (right): http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:52: * @param key A key that uniquely identifies this bitmap data (default: bitmap genID) On 2012/08/10 21:50:17, TomH wrote: > Do we ever need to use a non-default key? > Here again I want to bikeshed the name, but I guess it works. Probably not, I'd imagined there might be a case where we want to get data from somewhere other than a bitmap, but unless that becomes the case, the genID is always going to be the key, so no sense clogging up the API with an extra param. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:68: GrScalar getScaleFactor() { return SkIntToScalar(fRowHeight - 1) / fHeight; } On 2012/08/10 21:50:17, TomH wrote: > nit: const Fixed. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:103: * Helper that unlocks the texture On 2012/08/10 21:50:17, TomH wrote: > nit: is this comment really telling me anything? I'd written one for each of these functions since the majority of them ended up with comments and it looked odd to omit them from a few, but they really weren't adding anything, so I removed them. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:123: * Searches the key table for a key and returns the index (or ~(insertion index) if not found) On 2012/08/10 21:50:17, TomH wrote: > What is insertion index? I made this clearer. It's the index where you would insert the key to keep the list sorted (this is what SkTSearch returns). http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:129: * used unlocked row. Returns NULL otherwise. On 2012/08/10 21:50:17, TomH wrote: > Nit: runon sentence could be tighter. Fixed. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:147: // Used to uniquely identify our textures in the texture cache On 2012/08/10 21:50:17, TomH wrote: > Single static value uniquely identifying *multiple* textures? Bad wording on my part, fixed. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:156: GrContext* fContext; On 2012/08/10 21:50:17, TomH wrote: > We embed (approximately; see next comment) all the members of a desc - should we > just embed one instead? I could be convinced either way... I added a desc as a member, I'd originally thought it'd ugly up accessing the members, but it really doesn't hurt that much, and it slims up the constructor a fair bit. http://codereview.appspot.com/6457099/diff/10001/src/gpu/effects/GrTextureStr... src/gpu/effects/GrTextureStripAtlas.h:161: const int fWidth; On 2012/08/10 21:50:17, TomH wrote: > You use different types here than you use in a desc? I ended up just holding onto a desc instead of making copies of its members, so this is resolved. I'd originally used ints in both, but since we use the desc as a hash key with GrBinHashKey, and since the atlas dimensions shouldn't end up bigger than a 16-bit int can represent, I figured it'd be a waste to hash a bunch of extra bits that'd always be 0 anyways.
Sign in to reply to this message.
Bump, just in case this got lost in inboxes over the weekend...
Sign in to reply to this message.
I was *just about* to type... LGTM. Or maybe another long nitpicking review, but we'll go with LGTM for now.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/6457099/diff/9002/src/gpu/effects/GrTextureStri... File src/gpu/effects/GrTextureStripAtlas.cpp (right): http://codereview.appspot.com/6457099/diff/9002/src/gpu/effects/GrTextureStri... src/gpu/effects/GrTextureStripAtlas.cpp:89: if (row->fLocks == 0) { style nit: 0 == row->fLocks
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
