Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839833003:
Add medium image quality to software predecode. (Closed)
DescriptionAdd medium image quality to software predecode.
Software predecode has low and high qualities supported. This CL adds
support for medium quality.
BUG=594839
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/8b9662d8d9563da3ad545f1e7717d134238c8961
Cr-Commit-Position: refs/heads/master@{#391357}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Moving CLZ to a separate CL. Making changes as per CR comments. #
Total comments: 6
Patch Set 3 : Generating mip levels on demand. #
Total comments: 2
Patch Set 4 : Cherry picking from refactor CL. #Patch Set 5 : Cherry picking from updated refactor CL. #Patch Set 6 : Fixing bad rebase. #Patch Set 7 : Fixing unreachable code error. #Patch Set 8 : Using Skia's medium quality to generate the mipmap. #Patch Set 9 : Rebasing. #
Total comments: 6
Patch Set 10 : Adding mipmap scaling. #Patch Set 11 : Rebasing #
Total comments: 16
Patch Set 12 : Using SkSize instead of SkMatrix, some renaming + comments. #
Total comments: 7
Patch Set 13 : Comparing <= 0.f instead of == -1.f. Adding a lot of unit tests." #Patch Set 14 : Rebasing. #
Total comments: 15
Patch Set 15 : Now caching the mip level and using scale adjustment to get the actual destination size. #Patch Set 16 : Changing an error check to a DCHECK. Updating unit tests (including adding a new one) to reflect th… #
Total comments: 8
Patch Set 17 : Code review comments. #
Total comments: 6
Patch Set 18 : Adding a DCHECK. #Patch Set 19 : Fixing DCHECK. Removing unnecessary if. #Patch Set 20 : Removing return after NOTREACHED. #Patch Set 21 : Checking if this errors GCC/Clang. #Patch Set 22 : I assume this needs to be part of the macro but lets just check. #Patch Set 23 : I forgot I cannot nest macros. I'll just set it at the offending site. #Patch Set 24 : The warning only disables at function scope. #
Total comments: 4
Patch Set 25 : Removing NOTREACHED since VC was warning on it. Removing unneeded braces. #
Messages
Total messages: 117 (45 generated)
Description was changed from ========== [WIP] Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 ========== to ========== [WIP] Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== [WIP] Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== [WIP] Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
cblume@chromium.org changed reviewers: + vmpstr@chromium.org
cblume@chromium.org changed reviewers: + danakj@chromium.org
This is not yet ready. We still need to get Skia's thoughts on what they want to expose. I just wanted to lay some sort of groundwork. vmpstr: This creates a separate image to be cached for each mipmap level. That gives us the ability to attempt to fetch a specific mipmap level later on. danakj: While generating the mipmaps I wanted a count-leading-zeros function. I wrote one and added it to base/bits.h, including instrinsics supports where available on our compilers. I later realized I could perhaps use Log2Floor. But a better way might actually be to have Log2Floor use CLZ (including intrinsics). I wanted to get your thoughts.
https://codereview.chromium.org/1839833003/diff/1/base/bits.h File base/bits.h (right): https://codereview.chromium.org/1839833003/diff/1/base/bits.h#newcode52 base/bits.h:52: inline int CountLeadingZerosFallback(uint32_t value) { This should probably be a separate CL. Wdyt? https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... File cc/tiles/software_image_decode_controller.cc (left): https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:110: SkColorType SkColorTypeForDecoding(ResourceFormat format) { Why this change? https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:177: // If we're not going to do a scale, we will just create a task to preroll the \o/ https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:330: if (largest_axis > numeric_limits<uint32_t>::max()) { Do you know which systems we have that have a 64 bit int? I would prefer that this be a DCHECK or maybe even static_assert that sizeof(int) <= sizeof(uint32_t). I also don't think we'd be able to handle images that are larger thatn uint32_t max in one of the dimensions, but who knows. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:336: int significant_bits = (sizeof(uint32_t) * 8) - leading_zeros; sizeof(uint32_t) == 4 on all platforms that we support, I think? https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:337: // This is making the assumption taht the size of a byte is 8 bits s/taht/that/ I think that sizeof(char) is defined to be 1. and CHAR_BIT is the number of bits in a char. This comment could all be a static_assert. That being said, I'm kind of hesitant that we have to have this low level math in here. Is it possible to do something without relying on this? Like taking the mip dimension from the previous calculation and ending the iteration when we reach 0 in width/height? https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:341: scoped_ptr<DecodedImage> last_mip_scaled_image = nullptr; you don't need to initialize scoped_ptrs, they are by default nullptr. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:363: target_mip_image = last_mip_scaled_image; scoped_ptrs assign like this? last_mip_scaled_image is an lvalue. What is the outcome of this?
https://codereview.chromium.org/1839833003/diff/1/base/bits.h File base/bits.h (right): https://codereview.chromium.org/1839833003/diff/1/base/bits.h#newcode52 base/bits.h:52: inline int CountLeadingZerosFallback(uint32_t value) { On 2016/03/29 18:38:13, vmpstr wrote: > This should probably be a separate CL. Wdyt? I agree 100%. I'll separate it out. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... File cc/tiles/software_image_decode_controller.cc (left): https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:110: SkColorType SkColorTypeForDecoding(ResourceFormat format) { On 2016/03/29 18:38:13, vmpstr wrote: > Why this change? Oh sorry. I branched off my other branch after I had accidentally removed this from a merge. I'll put it back. (I already put it back on the other branch) https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:330: if (largest_axis > numeric_limits<uint32_t>::max()) { On 2016/03/29 18:38:13, vmpstr wrote: > Do you know which systems we have that have a 64 bit int? I would prefer that > this be a DCHECK or maybe even static_assert that sizeof(int) <= > sizeof(uint32_t). > > I also don't think we'd be able to handle images that are larger thatn uint32_t > max in one of the dimensions, but who knows. I agree with your comments. Actually, when I originally wrote it I had a DCHECK. But I thought to myself "this is a real error if it does happen" so I changed it. But I'm perfectly happy making it a DCHECK since it would require a few absurd things to both happen. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:333: int leading_zeros = SkCLZ(static_cast<uint32_t>(largest_axis)); Oops. I meant to make this the new CLZ function. Although, come to think of it, since we're depending on Skia anyway at this point, maybe we should just use Skia's? I think I'll leave it as Skia's for now. And I'll separate out the CLZ from base/bits.h. Then in that other CL I can have Log2Floor and the new CLZ both use intrinsics. If that lands I can update this to use the new CLZ instead of Skia's. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:336: int significant_bits = (sizeof(uint32_t) * 8) - leading_zeros; On 2016/03/29 18:38:13, vmpstr wrote: > sizeof(uint32_t) == 4 on all platforms that we support, I think? As far as I know, yes. Maybe make it a DCHECK and then hardcode 32 instead of sizeof(uint32_t) * 8? https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:337: // This is making the assumption taht the size of a byte is 8 bits On 2016/03/29 18:38:13, vmpstr wrote: > s/taht/that/ > > I think that sizeof(char) is defined to be 1. and CHAR_BIT is the number of bits > in a char. This comment could all be a static_assert. > > That being said, I'm kind of hesitant that we have to have this low level math > in here. Is it possible to do something without relying on this? Like taking the > mip dimension from the previous calculation and ending the iteration when we > reach 0 in width/height? Oh holy cow, I didn't know about CHAR_BIT. We could loop until both axes are 1. That would avoid the bit twiddling. I'll do that. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:341: scoped_ptr<DecodedImage> last_mip_scaled_image = nullptr; On 2016/03/29 18:38:13, vmpstr wrote: > you don't need to initialize scoped_ptrs, they are by default nullptr. Done. https://codereview.chromium.org/1839833003/diff/1/cc/tiles/software_image_dec... cc/tiles/software_image_decode_controller.cc:363: target_mip_image = last_mip_scaled_image; On 2016/03/29 18:38:13, vmpstr wrote: > scoped_ptrs assign like this? last_mip_scaled_image is an lvalue. What is the > outcome of this? The outcome is a build error. Whoops. Fixing.
I have made the changes mentioned. This build but has a run-time error (which is visible by running the unit tests). I just wanted to get early, WIP feedback. Are we happy with the way this would create a key for each mip level and cache them? I need to make another change so that it attempts to pull from the cache using a key from the correct mip level (instead of a key representing this exact scale). But once I do that it should create a key corresponding to a level in the mipmap stack so it can reuse that image if it is in the cache.
The part that would cache things is the thing that is calling the Medium/Low functions. So, I'm not sure why we would want to generate the full mip stack. It seems like a waste of time. I would also follow up with skia to see if they want to expose mip utilities. Specifically, skia normally would store all of this in one block of memory, which can be locked/unlocked. That will use more memory but have a better performance in the sense that when we switch levels, we don't have to do extra scaling. However, we need the ability to extract a specific level. https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:360: kMedium_SkFilterQuality, Passing medium here would make skia also generate mipmaps during scaling, no? I'm not sure what pixmap scalePixels would do with medium quality otherwise. https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:371: target_mip_image = std::move(last_mip_scaled_image); Should we just break here? https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:375: std::unique_ptr<DecodedImage>(mip_scaled_image.release()); is this just a std::move? https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:383: return make_scoped_ptr<DecodedImage>(target_mip_image.release()); just return target_mip_image;?
On 2016/03/29 21:48:04, vmpstr wrote: > The part that would cache things is the thing that is calling the Medium/Low > functions. So, I'm not sure why we would want to generate the full mip stack. It > seems like a waste of time. > > I would also follow up with skia to see if they want to expose mip utilities. > Specifically, skia normally would store all of this in one block of memory, > which can be locked/unlocked. That will use more memory but have a better > performance in the sense that when we switch levels, we don't have to do extra > scaling. However, we need the ability to extract a specific level. > > https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... > File cc/tiles/software_image_decode_controller.cc (right): > > https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... > cc/tiles/software_image_decode_controller.cc:360: kMedium_SkFilterQuality, > Passing medium here would make skia also generate mipmaps during scaling, no? > I'm not sure what pixmap scalePixels would do with medium quality otherwise. > > https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... > cc/tiles/software_image_decode_controller.cc:371: target_mip_image = > std::move(last_mip_scaled_image); > Should we just break here? > > https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... > cc/tiles/software_image_decode_controller.cc:375: > std::unique_ptr<DecodedImage>(mip_scaled_image.release()); > is this just a std::move? > > https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... > cc/tiles/software_image_decode_controller.cc:383: return > make_scoped_ptr<DecodedImage>(target_mip_image.release()); > just return target_mip_image;? A few questions: Couldn't we just always decode directly to the mip scale above our desired size, using high quality. This would be a more expensive scale, but would get us some of the re-use benefits. It would also avoid generating / re-generating parts of the mip chain when we switched levels. If we want to use bilinear for a cheaper scale (is this always cheaper?), it seems like we should cache the other mip levels as we generate them, unlocked, not just the final mip level. With the current approach it seems like we'd re-generate the whole mip tree each time a new mip level is used, which doesn't seem ideal.
>> With the current approach it seems like we'd re-generate the whole mip tree each time a new mip level is used, which doesn't seem ideal. The current approach is supposed to be (but isn't because it isn't finished): generate and cache them on first use. When a new level is requested, it will exit early by pulling it out of the cache before generating anything. I'm okay with either method (generate on demand or generate up front). I think I prefer the generate on demand method for memory reasons. Normally, you would be thinking about the base mip level (the largest). And all the others consume a bit more memory. But compared to the base level they aren't terrible. However, the case where mipmaps seem to be used most frequently are sites where users upload images and they are displayed in a gallery sort of way. In that case, the base mip level is rarely shown. And the base level is the one which is using most memory. So I feel like there may actually be a pretty significant memory win if we generate on demand. https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:360: kMedium_SkFilterQuality, On 2016/03/29 21:48:04, vmpstr wrote: > Passing medium here would make skia also generate mipmaps during scaling, no? > I'm not sure what pixmap scalePixels would do with medium quality otherwise. Yeah. This is sort of a placeholder until we figure out what we need from Skia. If we only want to generate 1 mip level then that changes significantly what we need from Skia. And I think I might like the idea of generating them on demand (as opposed to generating them all in advance). https://codereview.chromium.org/1839833003/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:371: target_mip_image = std::move(last_mip_scaled_image); On 2016/03/29 21:48:04, vmpstr wrote: > Should we just break here? If we are only generating one level (which I like), then yes.
vmpstr@chromium.org changed reviewers: + ericrk@chromium.org
https://codereview.chromium.org/1839833003/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:356: if (mip_height < key.target_size().height() || If the target size is 1x1, then I think this function would end up returning nullptr instead of the lowest mip level. On a somewhat related note, when you think this is approaching a good state, could you add some unittests?
I'm going to work on the unit tests next. I believe we're to a point where we need to get the mipmap filters exposed by skia. https://codereview.chromium.org/1839833003/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:356: if (mip_height < key.target_size().height() || On 2016/03/30 18:58:07, vmpstr wrote: > If the target size is 1x1, then I think this function would end up returning > nullptr instead of the lowest mip level. > > On a somewhat related note, when you think this is approaching a good state, > could you add some unittests? Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/140001
Description was changed from ========== [WIP] Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
cblume@chromium.org changed reviewers: - danakj@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/19 03:33:24, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. I think this is now ready for review. After the meeting with some Skia folks it was determined that using Skia's default medium filter may actually be best. It requires Skia to generate the entire mip tree. But we would probably want to generate all the levels down to the one we need anyway, since that might be faster. We can later measure if generating the unneeded smaller levels is actually worth adding some new API. The unit tests needed only minor updating and seem to be decently well covered. Because we are only caching the mip level requested (and not all mip levels) I don't think we need to add any tests which are missing. Let me know if you can think of something that we might want to test.
https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:544: decoded_pixmap.scalePixels(scaled_pixmap, key.filter_quality()); So... we don't actually cache the mip level in this case, but rather the precise scale? What caching happens on skia side? Do we cache the mips there? https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:258: std::unordered_set<uint32_t> prerolled_images_; This can go as well.
https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:544: decoded_pixmap.scalePixels(scaled_pixmap, key.filter_quality()); On 2016/04/19 23:04:57, vmpstr wrote: > So... we don't actually cache the mip level in this case, but rather the precise > scale? > > What caching happens on skia side? Do we cache the mips there? Oh, I made a mistake here. Skia does mipmapping internally and I thought (for some reason...wasn't paying attention) it would just return the mipmap level requested. Skia will return to you the exact scale, not the nearest mipmap. I'm fixing that. As far as caching, I would imagine it is the same as what happens with high quality. It looks like it maybe only caches the decoded_pixmap. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... makes it look like the scaled pixmap isn't cached. Why aren't we calling the 3-param version of .scalePixels which allows us to specify no caching? Right now caching is allowed (including with high quality). I don't believe Skia is caching the mip tree. Skia generates the mips inside SkMipMap. When that object is destroyed, it deallocates the mip levels. I don't think Skia is copying out each level from it, but I do not know that. I'll double check. https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:258: std::unordered_set<uint32_t> prerolled_images_; On 2016/04/19 23:04:57, vmpstr wrote: > This can go as well. Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/200001
Can you add a few unittests to verify that the scale/sizing is all correct? Specifically, requesting a medmium quality with image size say 500x200 and then requesting these scales: 1.5, 1.0, 0.75, 0.5, 0.49, 0.1, 0.01, 0.001 or something like that. I think it's worth verifying it for mipmap code, although I don't have similar verification for high quality (high quality is technically easier since it always scales to what you asked for) https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:544: decoded_pixmap.scalePixels(scaled_pixmap, key.filter_quality()); On 2016/04/21 19:28:11, cblume wrote: > On 2016/04/19 23:04:57, vmpstr wrote: > > So... we don't actually cache the mip level in this case, but rather the > precise > > scale? > > > > What caching happens on skia side? Do we cache the mips there? > > Oh, I made a mistake here. > Skia does mipmapping internally and I thought (for some reason...wasn't paying > attention) it would just return the mipmap level requested. Skia will return to > you the exact scale, not the nearest mipmap. I'm fixing that. > > As far as caching, I would imagine it is the same as what happens with high > quality. It looks like it maybe only caches the decoded_pixmap. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > makes it look like the scaled pixmap isn't cached. > > Why aren't we calling the 3-param version of .scalePixels which allows us to > specify no caching? Right now caching is allowed (including with high quality). Which 3 param version? I don't think SkPixmap has this. SkPixmap should not be caching anything when doing high filter quality, I was just unsure about mipmaps. > > I don't believe Skia is caching the mip tree. Skia generates the mips inside > SkMipMap. When that object is destroyed, it deallocates the mip levels. I don't > think Skia is copying out each level from it, but I do not know that. I'll > double check. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (left): https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:102: if (key.can_use_original_decode()) I'd kind of prefer if we keep this type of logic: if (foo) return blah; ... ... instead of if (!foo) { ... ... } return blah; but it's really up to you. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:125: keep whitespace https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:581: DCHECK_EQ(kHigh_SkFilterQuality, key.filter_quality()); Can you add a DCHECK(key.filter_quality() == kHigh_SkFilterQuality || k.filter_quality() == kMedium_SkFilterQuality)? https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:100: SkMatrix GetMipMapScale(const SoftwareImageDecodeController::ImageKey& key) { Can you add a comment here to describe what the function does and what it returns? Should this be return SkSize and then the caller builds a matrix out of it if they want? Maybe GetMipMapScaleAdjustment or something? https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:382: if (mipmap_scale.getScaleX() == 0 || mipmap_scale.getScaleY() == 0) { This should be doing an epsilon comparison since it's a float? https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:387: DrawImage mip_image(&image, gfx::RectToSkIRect(src_rect), nit: key.src_rect() directly here https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:389: auto mip_key = ImageKey::FromDrawImage(mip_image); Interesting, so you create a new DrawImage that is at the exact mipmap level and then use kMedium_SkFilterQuality but other than that it's going down the same path that high filter quality would go... That's pretty cool! Do we have guarantees that the mip size is exactly the same as what skia would pick? That is to say, would there still be a bilerp to adjust the size by one pixel or so maybe? https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:409: return DecodeImageMediumQuality(key, *image); To be consistent, can you have this GetMediumQualityImageDecode() Or something like GetMipMap...Decode? I don't know :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/220001
https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/160001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:544: decoded_pixmap.scalePixels(scaled_pixmap, key.filter_quality()); On 2016/04/21 20:11:30, vmpstr wrote: > On 2016/04/21 19:28:11, cblume wrote: > > On 2016/04/19 23:04:57, vmpstr wrote: > > > So... we don't actually cache the mip level in this case, but rather the > > precise > > > scale? > > > > > > What caching happens on skia side? Do we cache the mips there? > > > > Oh, I made a mistake here. > > Skia does mipmapping internally and I thought (for some reason...wasn't paying > > attention) it would just return the mipmap level requested. Skia will return > to > > you the exact scale, not the nearest mipmap. I'm fixing that. > > > > As far as caching, I would imagine it is the same as what happens with high > > quality. It looks like it maybe only caches the decoded_pixmap. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... > > makes it look like the scaled pixmap isn't cached. > > > > Why aren't we calling the 3-param version of .scalePixels which allows us to > > specify no caching? Right now caching is allowed (including with high > quality). > > Which 3 param version? I don't think SkPixmap has this. SkPixmap should not be > caching anything when doing high filter quality, I was just unsure about > mipmaps. > > > > > > I don't believe Skia is caching the mip tree. Skia generates the mips inside > > SkMipMap. When that object is destroyed, it deallocates the mip levels. I > don't > > think Skia is copying out each level from it, but I do not know that. I'll > > double check. > Oh sorry. I was looking at SkImage::scalePixels, which takes a 3rd param defaulting to allow caching. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (left): https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:102: if (key.can_use_original_decode()) On 2016/04/21 20:11:30, vmpstr wrote: > I'd kind of prefer if we keep this type of logic: > > if (foo) > return blah; > ... > ... > > instead of > > if (!foo) { > ... > ... > } > return blah; > > > but it's really up to you. Done. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:125: On 2016/04/21 20:11:31, vmpstr wrote: > keep whitespace Done. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:581: DCHECK_EQ(kHigh_SkFilterQuality, key.filter_quality()); On 2016/04/21 20:11:31, vmpstr wrote: > Can you add a > > DCHECK(key.filter_quality() == kHigh_SkFilterQuality || k.filter_quality() == > kMedium_SkFilterQuality)? Done. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:100: SkMatrix GetMipMapScale(const SoftwareImageDecodeController::ImageKey& key) { On 2016/04/21 20:11:30, vmpstr wrote: > Can you add a comment here to describe what the function does and what it > returns? > > Should this be return SkSize and then the caller builds a matrix out of it if > they want? > > Maybe GetMipMapScaleAdjustment or something? Done. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:382: if (mipmap_scale.getScaleX() == 0 || mipmap_scale.getScaleY() == 0) { On 2016/04/21 20:11:30, vmpstr wrote: > This should be doing an epsilon comparison since it's a float? This started a good conversation in my cube. Our consensus is that we don't need to compare against epsilon. But I want to provide the thought that got there: I actually changed it to -1 since close to 0 is a possible scale. I wanted to get away from any possibly-good value. -1 can be represented perfectly in floating point, no matter the size of the float. There must always be at least 1 sign bit and 1 mantissa bit. However, there were other things discussed which are important to note. First, there were no operations to get to -1. It was a literal. If there were operations, we would need the epsilon. Additionally, there is a case where the number couldn't be perfectly represented like say comparing -1.3f == -1.3f. The compiler will generate sort of literal. But it is possible that the compiler generates say a 80-bit literal. When the original literal was stored into a float, it will involve rounding from the 80-bits. Now then, all that said, didn't we just approve std::optional? In general I dislike invalid values... https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:387: DrawImage mip_image(&image, gfx::RectToSkIRect(src_rect), On 2016/04/21 20:11:31, vmpstr wrote: > nit: key.src_rect() directly here Done. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:389: auto mip_key = ImageKey::FromDrawImage(mip_image); On 2016/04/21 20:11:30, vmpstr wrote: > Interesting, so you create a new DrawImage that is at the exact mipmap level and > then use kMedium_SkFilterQuality but other than that it's going down the same > path that high filter quality would go... That's pretty cool! > > Do we have guarantees that the mip size is exactly the same as what skia would > pick? That is to say, would there still be a bilerp to adjust the size by one > pixel or so maybe? I asked the Skia team about if it runs a bilerp if I requested the exact scale of the mipmap. I was told it would not. Unfortunately, I do not have a good way to guarantee that the mip size is exactly the same as the one used by Skia. That would require Skia exporting such information. So I cannot DCHECK it or the likes. Right now, I just happen to know that medium quality is mipmaps and I happen to know Skia uses SkMipMap. And I happen to know SkMipMap fairly well. A better solution would probably be to export SkMipMap, but that requires exporting other dependencies as well. SkMipMap allows access to each mip level, including that mip level's size. https://codereview.chromium.org/1839833003/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:409: return DecodeImageMediumQuality(key, *image); On 2016/04/21 20:11:30, vmpstr wrote: > To be consistent, can you have this GetMediumQualityImageDecode() > > Or something like GetMipMap...Decode? I don't know :) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good. Can you add the unittests I mentioned in #38 https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:387: // So add 1 and compare to epsilon. Comment is incorrect https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:388: if (mipmap_scale.width() == -1.f || mipmap_scale.height() == -1.f) { I understand your reasoning, but I think changing this to mipmap_scale.width() <= 0 || mipmap_scale.height() <= 0 does not hurt performance? base::Optional is available, but if you want to go that route then I would prefer that the function returns a bool and takes the scale by pointer as an extra argument.
https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:123: next_mip_width < key.target_size().width()) { I think this hits an edge case if you actually want the 1x1 mip level - next_mip_height and next_mip_width can never be less than 1, so if the target is 1x1, this will never be true, and we will always return the invalid value (-1,-1)
On 2016/04/22 18:17:10, vmpstr wrote: > Looks good. Can you add the unittests I mentioned in #38 Yeah, sorry. Forgot to add that bit. Doing it now.
https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:388: if (mipmap_scale.width() == -1.f || mipmap_scale.height() == -1.f) { On 2016/04/22 18:17:10, vmpstr wrote: > I understand your reasoning, but I think changing this to > > mipmap_scale.width() <= 0 || mipmap_scale.height() <= 0 > > does not hurt performance? base::Optional is available, but if you want to go > that route then I would prefer that the function returns a bool and takes the > scale by pointer as an extra argument. Also... The situation in which this can happen is only when key.target_size() width/height is 0, right? In those situations, we already would have skipped the image (ie there are a few target_size().IsEmpty() checks in this code). Can this be a DCHECK instead?
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/260001
https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:123: next_mip_width < key.target_size().width()) { On 2016/04/23 00:25:48, ericrk wrote: > I think this hits an edge case if you actually want the 1x1 mip level - > next_mip_height and next_mip_width can never be less than 1, so if the target is > 1x1, this will never be true, and we will always return the invalid value > (-1,-1) Done. https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:387: // So add 1 and compare to epsilon. On 2016/04/22 18:17:10, vmpstr wrote: > Comment is incorrect Done. https://codereview.chromium.org/1839833003/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:388: if (mipmap_scale.width() == -1.f || mipmap_scale.height() == -1.f) { On 2016/04/22 18:17:10, vmpstr wrote: > I understand your reasoning, but I think changing this to > > mipmap_scale.width() <= 0 || mipmap_scale.height() <= 0 > > does not hurt performance? base::Optional is available, but if you want to go > that route then I would prefer that the function returns a bool and takes the > scale by pointer as an extra argument. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:124: SkScalar y_scale = 1.f; nit: i'd just do the math below unconditionally, but up to you. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:140: return SkSize::Make(-1.f, -1.f); Should this be a NOTREACHED? Is this code called with arguments that fall in here? https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:148: } else { nit: } else if (...) { } else { } https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:413: // So add 1 and compare to epsilon. Comment is still wrong :) https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:414: if (mipmap_scale.width() <= 0.f || mipmap_scale.height() <= 0.f) { Can this be a DCHECK instead? That is, are there cases where we call this function with an image that causes a <= 0 scale? https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:445: return GetMediumQualityImageDecode(key, std::move(image)); I'm a bit concerned about the fact that key and key generation didn't change here. Would this mean that if we have an image I that appears at medium quality in two places at scale 0.4 and 0.45, then we'd cache it twice: key (I, 0.4) -> mip at scale 0.5 key (I, 0.45) -> mip at scale 0.5 That seems wasteful since it's the same mip. Or am I missing something? https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller_unittest.cc (right): https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller_unittest.cc:1274: TEST(SoftwareImageDecodeControllerTest, MediumQualityAt0_49ScaleIsHandled) { Can you combine something like this test and the one above into one and also add EXPECT_EQ(decoded_draw_image49.image(), decoded_draw_image50.image())? I think that should be a requirement. WDYT?
https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:124: SkScalar y_scale = 1.f; On 2016/04/29 19:09:41, vmpstr wrote: > nit: i'd just do the math below unconditionally, but up to you. I thought I had run into a situation where this caused a not-identity case earlier. But it is passing all the tests now, which check for identity when it should be. So perhaps my non-identity problem was just my bad code. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:140: return SkSize::Make(-1.f, -1.f); On 2016/04/29 19:09:40, vmpstr wrote: > Should this be a NOTREACHED? Is this code called with arguments that fall in > here? I changed it to NOTREACHED but in our unit test of scale 0.001 it actually hits this. The requested scale is smaller than an image can actually store. (IE 0 height or width.) So I took the NOTREACHED back out and added the error checking for if it was a bad call. Should I instead change it to be NOTREACHED and check the params before making the call? I think that might be how we normally do it on Chrome. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:148: } else { On 2016/04/29 19:09:40, vmpstr wrote: > nit: > > } else if (...) { > } else { > } Done. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:413: // So add 1 and compare to epsilon. On 2016/04/29 19:09:40, vmpstr wrote: > Comment is still wrong :) Oops. I could have sworn I fixed that. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:414: if (mipmap_scale.width() <= 0.f || mipmap_scale.height() <= 0.f) { On 2016/04/29 19:09:40, vmpstr wrote: > Can this be a DCHECK instead? That is, are there cases where we call this > function with an image that causes a <= 0 scale? This is similar to the NOTREACHED thing above. When the requested scale turns the height or width into 0, we get this bad case and need to actually check for it. I could instead check early if the requested height or width would be 0. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:445: return GetMediumQualityImageDecode(key, std::move(image)); On 2016/04/29 19:09:40, vmpstr wrote: > I'm a bit concerned about the fact that key and key generation didn't change > here. > > Would this mean that if we have an image I that appears at medium quality in two > places at scale 0.4 and 0.45, then we'd cache it twice: > > key (I, 0.4) -> mip at scale 0.5 > key (I, 0.45) -> mip at scale 0.5 > > That seems wasteful since it's the same mip. Or am I missing something? You are completely correct. This should be fixed now.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:414: if (mipmap_scale.width() <= 0.f || mipmap_scale.height() <= 0.f) { On 2016/04/29 19:09:40, vmpstr wrote: > Can this be a DCHECK instead? That is, are there cases where we call this > function with an image that causes a <= 0 scale? I switched this over to a DCHECK since it is separate form the ::FromImageKey and may not ever get a scale that produces a 0 height or width. It seems to work. Please double check me on this. https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller_unittest.cc (right): https://codereview.chromium.org/1839833003/diff/260001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller_unittest.cc:1274: TEST(SoftwareImageDecodeControllerTest, MediumQualityAt0_49ScaleIsHandled) { On 2016/04/29 19:09:41, vmpstr wrote: > Can you combine something like this test and the one above into one and also add > > EXPECT_EQ(decoded_draw_image49.image(), decoded_draw_image50.image())? > > I think that should be a requirement. WDYT? Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:109: int target_width = target_size.width(); nit: you can early out if target_width or target_height are 0 (return (-1, -1), and then NOTREACHED at the end). This works as well though. https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:132: if (target_height == 1 && target_width == 1) { This is kind of meh... Do you still need this if you early out before? https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:422: auto mip_key = ImageKey::FromDrawImage(mip_image); I'm a bit confused: wouldn't the passed key already have the right target_width/height? https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:882: if (can_use_original_decode && !target_size.IsEmpty()) Should the below condition also check if target_size is empty?
https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:109: int target_width = target_size.width(); On 2016/05/02 19:58:26, vmpstr wrote: > nit: you can early out if target_width or target_height are 0 (return (-1, -1), > and then NOTREACHED at the end). This works as well though. I had tried this but still needed a final return, or else I would get an error about control possibly reaching the end of the function. It does offer us an early out though. But that is favoring the rare error case and introducing a branch for all the good cases. I'll go ahead and add it so you can take a look and see if you like it more. https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:132: if (target_height == 1 && target_width == 1) { On 2016/05/02 19:58:26, vmpstr wrote: > This is kind of meh... Do you still need this if you early out before? This is for the final mip level case (say 1x1), not the 0 height/width error case. It is tricky. I previously was doing L123 "if (next_mip_height < target_height..." and no check on this line. It would get to the final mip level, see we haven't hit our target yet, and return an error. So it wasn't handling 1x1. My first attempt to handle the 1x1 case was to change L123 to <=. The problem with that is say we have a 500x500 image that we want to present at 250x250. Using the first mip level (not the base level) gives us exactly what we want. But this would use the 500x500 image, instead of the 250x250 image. So I arrived at this solution. I think it actually handles the 1x1 case pretty well (or 1xM or Nx1). Only after we've scaled all the way down does it consider this branch. An alternative would be know in advance how many mip levels we need. Then we don't need a special case for the final mip level. My original design had this. We didn't like it because it relied on some bit manipulation math. https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:422: auto mip_key = ImageKey::FromDrawImage(mip_image); On 2016/05/02 19:58:26, vmpstr wrote: > I'm a bit confused: wouldn't the passed key already have the right > target_width/height? Good catch. After our change to ::FromDrawImage, this always already had the right scale. This is about to get CLEAN yo! https://codereview.chromium.org/1839833003/diff/300001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:882: if (can_use_original_decode && !target_size.IsEmpty()) On 2016/05/02 19:58:26, vmpstr wrote: > Should the below condition also check if target_size is empty? Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/320001
lgtm with nits https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:135: if (target_height == 1 && target_width == 1) { nit: don't need this if anymore https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:871: GetMipMapScaleAdjustment(src_rect.size(), target_size); nit: dcheck mipmap scale != -1?
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:135: if (target_height == 1 && target_width == 1) { On 2016/05/02 23:13:16, vmpstr wrote: > nit: don't need this if anymore Wait, I think we might, right? This isn't for 1.0x scale. This is for the mipmap level that is 1x1. https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:871: GetMipMapScaleAdjustment(src_rect.size(), target_size); On 2016/05/02 23:13:16, vmpstr wrote: > nit: dcheck mipmap scale != -1? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/340001
https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:135: if (target_height == 1 && target_width == 1) { On 2016/05/02 23:35:14, cblume wrote: > On 2016/05/02 23:13:16, vmpstr wrote: > > nit: don't need this if anymore > > Wait, I think we might, right? > This isn't for 1.0x scale. > This is for the mipmap level that is 1x1. Right, it has to be 1x1 here, since if it's > then we would've returned already and if it's 0x1 then we would've earlied out earlier.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/320001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:135: if (target_height == 1 && target_width == 1) { On 2016/05/02 23:39:06, vmpstr wrote: > On 2016/05/02 23:35:14, cblume wrote: > > On 2016/05/02 23:13:16, vmpstr wrote: > > > nit: don't need this if anymore > > > > Wait, I think we might, right? > > This isn't for 1.0x scale. > > This is for the mipmap level that is 1x1. > > Right, it has to be 1x1 here, since if it's > then we would've returned already > and if it's 0x1 then we would've earlied out earlier. Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2016/05/03 00:40:32, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) VS software_image_decode_controller.cc(142) : warning C4702: unreachable code (warnings treated as errors) That line of code is NOTREACHED(); Why do you hate me, Chrome?
On Mon, May 2, 2016 at 6:26 PM, <cblume@chromium.org> wrote: > On 2016/05/03 00:40:32, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp... > ) > > VS software_image_decode_controller.cc(142) : warning C4702: unreachable > code > (warnings treated as errors) > > That line of code is NOTREACHED(); > > Why do you hate me, Chrome? > Your for loop is unable to exit, I don't think you need anything after it. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1839833003/diff/460001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/460001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:104: #if defined(_MSC_VER) nit: let's just remove all of this and remove NOTREACHED in this case, since it's impossible to hit it (ie it doesn't depend on the input) https://codereview.chromium.org/1839833003/diff/460001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:118: if (target_height == 0 || target_width == 0) { nit: remove braces.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
https://codereview.chromium.org/1839833003/diff/460001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1839833003/diff/460001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:104: #if defined(_MSC_VER) On 2016/05/03 18:23:21, vmpstr wrote: > nit: let's just remove all of this and remove NOTREACHED in this case, since > it's impossible to hit it (ie it doesn't depend on the input) Done. https://codereview.chromium.org/1839833003/diff/460001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:118: if (target_height == 0 || target_width == 0) { On 2016/05/03 18:23:21, vmpstr wrote: > nit: remove braces. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by cblume@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839833003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839833003/480001
Message was sent while issue was closed.
Description was changed from ========== Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add medium image quality to software predecode. Software predecode has low and high qualities supported. This CL adds support for medium quality. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/8b9662d8d9563da3ad545f1e7717d134238c8961 Cr-Commit-Position: refs/heads/master@{#391357} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/8b9662d8d9563da3ad545f1e7717d134238c8961 Cr-Commit-Position: refs/heads/master@{#391357} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
