Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042133002:
Add display-resolution caching to GPU IDC (Closed)
DescriptionAdd display-resolution caching to GPU IDC
In order to save memory and upload time, images which are
going to be drawn at less than their native resolution may
be uploaded at a smaller scale. This change adds additional
caching and upload logic to allow this.
See https://goo.gl/0zCd9Z for a complete description.
BUG=623688
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel
Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168
Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3
Cr-Original-Commit-Position: refs/heads/master@{#402377}
Cr-Commit-Position: refs/heads/master@{#402694}
Patch Set 1 #Patch Set 2 : hi #Patch Set 3 : comments #
Total comments: 16
Patch Set 4 : Cleanup/feedback #
Total comments: 30
Patch Set 5 : Add display-resolution caching to GPU IDC #
Total comments: 14
Patch Set 6 : feedback #Patch Set 7 : link to doc #Patch Set 8 : use link shortener #Patch Set 9 : remove emplace #Patch Set 10 : Fix negative values + add rebaselines #Patch Set 11 : rebase #Patch Set 12 : combine with crrev.com/2103353002 #
Messages
Total messages: 83 (48 generated)
Description was changed from ========== Add display-resolution caching to GPU IDC Currently, the GPU BUG= ========== to ========== Add display-resolution caching to GPU IDC Currently, the GPU BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
ericrk@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.h (right): https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:69: // saved even when their ref-count reaches zero to allow for future re-use. It would be good to mention that it's kept there assuming that the budget isn't breached (ie this image_data_ is governed by the specified memory limit) https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:222: const scoped_refptr<ImageData>& image_data); Pass by value, move into place? https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:223: ImageDataForDrawImageEntry(const ImageDataForDrawImageEntry&); nit: can you name the parameters? "other"? Also, does the compiler complain if they are = default inline? https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:234: uint32_t image_id; = 0 here and below https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:247: base::HashInts(key.mip_level, key.quality)); I wonder if you can do something smarter than HashInts, since you know the magnitude of possible values here. (HashInts might be slow for these, but I don't think it matters that much). https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:281: // Returns true if the given ImageData is as large or larger than the This comment reads like like this could be an out of line function, but I guess you want it be able to access private structs? https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:288: DrawImageKey GenerateDrawImageKey(const DrawImage& draw_image) const; This needs a better comment; specifically, can you mention the difference between what each function does? Also, maybe one can be Generate if it creates a new thing, and Find if looks something up? https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:313: DrawImageMap<ImageDataForDrawImageEntry> image_data_for_draw_image_; I'd find better names for these caches, maybe something like unique_id_cache_ draw_image_cache_ or maybe something else?
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Ok, cleaned this up, thanks for the feedback! https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.h (right): https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:69: // saved even when their ref-count reaches zero to allow for future re-use. On 2016/06/14 21:35:27, vmpstr wrote: > It would be good to mention that it's kept there assuming that the budget isn't > breached (ie this image_data_ is governed by the specified memory limit) Done. https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:222: const scoped_refptr<ImageData>& image_data); On 2016/06/14 21:35:27, vmpstr wrote: > Pass by value, move into place? Done. https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:223: ImageDataForDrawImageEntry(const ImageDataForDrawImageEntry&); On 2016/06/14 21:35:27, vmpstr wrote: > nit: can you name the parameters? "other"? > > Also, does the compiler complain if they are = default inline? sadness: error: [chromium-style] Complex constructor has an inlined body. https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:234: uint32_t image_id; On 2016/06/14 21:35:27, vmpstr wrote: > = 0 here and below Done. https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:247: base::HashInts(key.mip_level, key.quality)); On 2016/06/14 21:35:27, vmpstr wrote: > I wonder if you can do something smarter than HashInts, since you know the > magnitude of possible values here. (HashInts might be slow for these, but I > don't think it matters that much). Sure, I'll make a better hash :P https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:281: // Returns true if the given ImageData is as large or larger than the On 2016/06/14 21:35:28, vmpstr wrote: > This comment reads like like this could be an out of line function, but I guess > you want it be able to access private structs? Now that the key is a uint64_t, moved this out-of-line. https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:288: DrawImageKey GenerateDrawImageKey(const DrawImage& draw_image) const; On 2016/06/14 21:35:27, vmpstr wrote: > This needs a better comment; specifically, can you mention the difference > between what each function does? Also, maybe one can be Generate if it creates a > new thing, and Find if looks something up? Removed the second fn and cleaned this up. https://codereview.chromium.org/2042133002/diff/100001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:313: DrawImageMap<ImageDataForDrawImageEntry> image_data_for_draw_image_; On 2016/06/14 21:35:27, vmpstr wrote: > I'd find better names for these caches, maybe something like > > unique_id_cache_ > draw_image_cache_ > > or maybe something else? Renamed to |persistent_cache_| and |in_use_cache_| to represent the data they store. WDYT?
Description was changed from ========== Add display-resolution caching to GPU IDC Currently, the GPU BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add display-resolution caching to GPU IDC BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:60: // Calculate the mp level to pre-scale the image to before uploading. We use mip s/mp/mip/ https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:62: int CalculatePreScaleMipLevel(const DrawImage& draw_image) { Can we rename "Pre" to "Upload" https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:64: // Images which are being clipped will have color-bleeding if scaled. Move the comment before the if, please https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:80: SkSize CalculatePreScaleFactor(const DrawImage& draw_image, int mip_level) { Put a comment before the function please https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:239: // ═══════════╪═══════╪══════════════════ nice! https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:620: // Upload task is complete, remove our referene to it. s/ne/nce/ https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:704: .insert(std::make_pair(key, InUseCacheEntry(found_image->second))) make_pair here will make a std::pair<InUseCacheKey, CacheEntry> and then insert that into the map, which expects a std::pair<const InUseCacheKey, CacheEntry> which makes a copy. Can we use emplace? I think it might work but might not compile on some systems, try it out? or decltype(in_use_cache_)::value_type(key, ...) https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1068: auto found = in_use_cache_.find(GenerateInUseCacheKey(draw_image)); I prefer if you just name a variable a different thing and remove the extra scopes https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1069: if (found != in_use_cache_.end()) { no braces https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1082: RefCountChanged(found->second.get()); Where did we change the ref count? Can this be currently used during raster? I'm not sure I understand the relationship here and why we can erase things. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1098: bool not_scaled = image_data->pre_scale_mip_level == 0; can you name bools in the positive? ie "is_scaled" and then the if check can be !is_scaled https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.h (right): https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:55: // RASTER-SCALE CACHING: Can you move this to a doc? Can you add a section to it about ref counts? Some things are ref counted in the "scoped_refptr" sense and some are ref counted in the "int ref_count" sense, which makes it a bit unclear. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:145: // 2b: 0.5x scale is about to be replaced by 1x scale, remove it from the When does this happen? What about the case where the 0.5x raster is going to run before 1x decode has a chance to run? https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:313: bool is_orphaned = false; Set the above variables as well (even though they're overridden by the ctor) https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:370: // first in the |in_use_cache_|, and then in the |persistent_cache_|. When would it check the persistent_cache? From your comment, I'm under the impression that there would always be a persistent_cache entry? Or do you mean this function can be used to populate persistent_cache?
Thanks for the feedback! https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:60: // Calculate the mp level to pre-scale the image to before uploading. We use mip On 2016/06/21 20:07:08, vmpstr wrote: > s/mp/mip/ Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:62: int CalculatePreScaleMipLevel(const DrawImage& draw_image) { On 2016/06/21 20:07:08, vmpstr wrote: > Can we rename "Pre" to "Upload" Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:64: // Images which are being clipped will have color-bleeding if scaled. On 2016/06/21 20:07:07, vmpstr wrote: > Move the comment before the if, please Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:80: SkSize CalculatePreScaleFactor(const DrawImage& draw_image, int mip_level) { On 2016/06/21 20:07:07, vmpstr wrote: > Put a comment before the function please Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:239: // ═══════════╪═══════╪══════════════════ On 2016/06/21 20:07:08, vmpstr wrote: > nice! Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:620: // Upload task is complete, remove our referene to it. On 2016/06/21 20:07:08, vmpstr wrote: > s/ne/nce/ Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:704: .insert(std::make_pair(key, InUseCacheEntry(found_image->second))) On 2016/06/21 20:07:08, vmpstr wrote: > make_pair here will make a std::pair<InUseCacheKey, CacheEntry> and then insert > that into the map, which expects a std::pair<const InUseCacheKey, CacheEntry> > which makes a copy. > > Can we use emplace? I think it might work but might not compile on some systems, > try it out? > > or decltype(in_use_cache_)::value_type(key, ...) Emplace works on windows - will update this again if it fails on other platforms. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1068: auto found = in_use_cache_.find(GenerateInUseCacheKey(draw_image)); On 2016/06/21 20:07:08, vmpstr wrote: > I prefer if you just name a variable a different thing and remove the extra > scopes Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1069: if (found != in_use_cache_.end()) { On 2016/06/21 20:07:08, vmpstr wrote: > no braces Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1082: RefCountChanged(found->second.get()); On 2016/06/21 20:07:07, vmpstr wrote: > Where did we change the ref count? Can this be currently used during raster? I'm > not sure I understand the relationship here and why we can erase things. RefCountChanged really now means "ownership changed" or something like that... updated the name. We just orphaned a task in the persistent cache - if it has 0 references, we will delete it in the line below. In order to ensure that we clean up SkImages and byte counts, we need to call RefCountChanged/OwnershipChanged before deleting it. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:1098: bool not_scaled = image_data->pre_scale_mip_level == 0; On 2016/06/21 20:07:08, vmpstr wrote: > can you name bools in the positive? ie "is_scaled" and then the if check can be > !is_scaled Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.h (right): https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:55: // RASTER-SCALE CACHING: On 2016/06/21 20:07:08, vmpstr wrote: > Can you move this to a doc? Can you add a section to it about ref counts? Some > things are ref counted in the "scoped_refptr" sense and some are ref counted in > the "int ref_count" sense, which makes it a bit unclear. Added a ref-counting section. Will move to a doc once review is complete and we're happy with the contents (rather than reviewing in two places). https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:145: // 2b: 0.5x scale is about to be replaced by 1x scale, remove it from the On 2016/06/21 20:07:08, vmpstr wrote: > When does this happen? What about the case where the 0.5x raster is going to run > before 1x decode has a chance to run? The 0.5x scale is only removed from the persistent cache, not the in use cache - the 0.5x draw will use the entry from the in use cache. Updated the comment to reflect this. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:313: bool is_orphaned = false; On 2016/06/21 20:07:08, vmpstr wrote: > Set the above variables as well (even though they're overridden by the ctor) Done. https://codereview.chromium.org/2042133002/diff/160001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:370: // first in the |in_use_cache_|, and then in the |persistent_cache_|. On 2016/06/21 20:07:08, vmpstr wrote: > When would it check the persistent_cache? From your comment, I'm under the > impression that there would always be a persistent_cache entry? Or do you mean > this function can be used to populate persistent_cache? If the image has previously been used, but is not currently in-use, it won't have an in_use_cache_ entry, but will have a persistent_cache_ entry. The in_use_cache entry is generated the first time we AddRef an ImageData (if it isn't already in the in_use_cache_)
https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:68: if (draw_image.src_rect() != draw_image.image()->bounds()) { nit: no braces https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:396: } else { nit: remove https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:403: if (!image_data) { Can you restructure the code from 369 to 405 as: scoped_refptr<ImageData> new_data; ImageData* image_data = GetImageDataForDrawImage(draw_image); if (!image_data) { ... } else if (image_data->is_at_raster) { ... } else if (image_data->decode.decode_failure) { ... } etc? https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:500: TRACE_EVENT0("cc", "GpuImageDecodeController::DrawWithImageFinished"); nit: Move this before the early out https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:926: std::max(kMedium_SkFilterQuality, draw_image.filter_quality()); Based on the comment, this should be std::min https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.h (right): https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:336: bool is_orphaned = false; Add a comment here about what orphaned is https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:401: bool IsCompatibleWithDrawImage(const ImageData* image_data, I think "IsCompatible" is good enough, but maybe this is more clear. Up to you.
Thanks for the feedback. Addressed comments and moved the example comments to a publicly shared google doc. https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:68: if (draw_image.src_rect() != draw_image.image()->bounds()) { On 2016/06/22 21:33:34, vmpstr wrote: > nit: no braces Done. https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:396: } else { On 2016/06/22 21:33:35, vmpstr wrote: > nit: remove Done. https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:403: if (!image_data) { On 2016/06/22 21:33:35, vmpstr wrote: > Can you restructure the code from 369 to 405 as: > > scoped_refptr<ImageData> new_data; > ImageData* image_data = GetImageDataForDrawImage(draw_image); > if (!image_data) { > ... > } else if (image_data->is_at_raster) { > ... > } else if (image_data->decode.decode_failure) { > ... > } > > etc? good call. https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:500: TRACE_EVENT0("cc", "GpuImageDecodeController::DrawWithImageFinished"); On 2016/06/22 21:33:35, vmpstr wrote: > nit: Move this before the early out I also log "GetDecodedImageForDraw" after the early out, I'll move both of these. https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.cc:926: std::max(kMedium_SkFilterQuality, draw_image.filter_quality()); On 2016/06/22 21:33:34, vmpstr wrote: > Based on the comment, this should be std::min definitely :D - Re-structured the code a bit to put this logic in a common function that can be used multiple places. Also added a unit test that tests one of these (others are difficult to unit test). https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... File cc/tiles/gpu_image_decode_controller.h (right): https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:336: bool is_orphaned = false; On 2016/06/22 21:33:35, vmpstr wrote: > Add a comment here about what orphaned is Done. https://codereview.chromium.org/2042133002/diff/180001/cc/tiles/gpu_image_dec... cc/tiles/gpu_image_decode_controller.h:401: bool IsCompatibleWithDrawImage(const ImageData* image_data, On 2016/06/22 21:33:35, vmpstr wrote: > I think "IsCompatible" is good enough, but maybe this is more clear. Up to you. sure.
lgtm thanks!
The CQ bit was checked by ericrk@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/2042133002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2042133002/#ps230005 (title: "remove emplace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Description was changed from ========== Add display-resolution caching to GPU IDC BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add display-resolution caching to GPU IDC BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ==========
Description was changed from ========== Add display-resolution caching to GPU IDC BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ==========
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ==========
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ==========
Patchset #10 (id:270001) has been deleted
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Fixed an issue where we were not handling negative image dimensions. Added a unit test to verify that we handle this case.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ericrk@chromium.org
Patchset #10 (id:290001) has been deleted
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2042133002/#ps310001 (title: "Fix negative values + add rebaselines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #10 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:310001) has been created in https://codereview.chromium.org/2101403006/ by ericrk@chromium.org. The reason for reverting is: This caused stability issues. Reverting until fix is in place..
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377} ==========
Patchset #11 (id:330001) has been deleted
Patchset #11 (id:350001) has been deleted
The CQ bit was checked by ericrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #12 (id:390001) has been deleted
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2042133002/#ps410001 (title: "combine with crrev.com/2103353002")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:410001)
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Cr-Commit-Position: refs/heads/master@{#402377} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Commit-Position: refs/heads/master@{#402694}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:410001) has been created in https://codereview.chromium.org/2107883003/ by shans@chromium.org. The reason for reverting is: Pretty sure this is causing: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6... (and same problem on Mac10.9, 10.10, 10.11).
Message was sent while issue was closed.
Patchset #12 (id:410001) has been deleted
Message was sent while issue was closed.
Patchset #11 (id:370001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ==========
On 2016/06/29 03:58:31, shans wrote: > A revert of this CL (patchset #12 id:410001) has been created in > https://codereview.chromium.org/2107883003/ by mailto:shans@chromium.org. > > The reason for reverting is: Pretty sure this is causing: > > https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/6... > > (and same problem on Mac10.9, 10.10, 10.11). Had a racy merge with rebaseline bot (my bad, should have expected this). Fixed and re-landing.
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2042133002/#ps450001 (title: "combine with crrev.com/2103353002")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:450001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Committed: https://crrev.com/e38a1f76d3e72234dee77aa9053299cab2e4f549 Cr-Original-Original-Commit-Position: refs/heads/master@{#402377} Cr-Original-Commit-Position: refs/heads/master@{#402694} Cr-Commit-Position: refs/heads/master@{#402921} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e38a1f76d3e72234dee77aa9053299cab2e4f549 Cr-Commit-Position: refs/heads/master@{#402921}
Message was sent while issue was closed.
Description was changed from ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Committed: https://crrev.com/e38a1f76d3e72234dee77aa9053299cab2e4f549 Cr-Original-Original-Commit-Position: refs/heads/master@{#402377} Cr-Original-Commit-Position: refs/heads/master@{#402694} Cr-Commit-Position: refs/heads/master@{#402921} ========== to ========== Add display-resolution caching to GPU IDC In order to save memory and upload time, images which are going to be drawn at less than their native resolution may be uploaded at a smaller scale. This change adds additional caching and upload logic to allow this. See https://goo.gl/0zCd9Z for a complete description. BUG=623688 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:mac_blink_rel;tryserver.blink:win_blink_rel Committed: https://crrev.com/bc8acf2632042e5e93982088d5b7dcb7f1c0d168 Committed: https://crrev.com/af0093b301058e5b06ea8a2e84dcf5071545a2b3 Cr-Original-Commit-Position: refs/heads/master@{#402377} Cr-Commit-Position: refs/heads/master@{#402694} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
