|
|
Created:
5 years ago by Stephen White Modified:
5 years ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Restore GPU suitability check in cached_picture case.
BUG=571033
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/8d842943f35e5be9cb5607dc948aafd600f018cb
Cr-Commit-Position: refs/heads/master@{#366226}
Patch Set 1 #Patch Set 2 : add unit tests #
Messages
Total messages: 23 (7 generated)
Description was changed from ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 ========== to ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
senorblanco@chromium.org changed reviewers: + enne@chromium.org
enne@: PTAL. Thanks! (This should be covered by DisplayItemListTest, IsSuitableForGpuRasterizationWithCachedPicture, but it isn't failing in ToT for some reason.)
On Fri, Dec 18, 2015 at 10:16 AM, <senorblanco@chromium.org> wrote: > Reviewers: enne > CL: https://codereview.chromium.org/1538913002/ > > Message: > enne@: PTAL. Thanks! > > (This should be covered by DisplayItemListTest, > IsSuitableForGpuRasterizationWithCachedPicture, but it isn't failing in > ToT for > some reason.) > Can you fix the test please? > > Description: > cc: Restore GPU suitability check in cached_picture case. > > BUG=571033 > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+2, -0 lines): > M cc/playback/display_item_list.cc > > > Index: cc/playback/display_item_list.cc > diff --git a/cc/playback/display_item_list.cc > b/cc/playback/display_item_list.cc > index > 65f36377aa448be11ba41fc1bd098f3af3eb63d6..80c713d8a6514bf98658565f96c6c2e5ca5dc842 > 100644 > --- a/cc/playback/display_item_list.cc > +++ b/cc/playback/display_item_list.cc > @@ -183,6 +183,8 @@ void DisplayItemList::Finalize() { > SkPictureUtils::ApproximateBytesUsed(picture_.get()); > recorder_.reset(); > canvas_.clear(); > + is_suitable_for_gpu_rasterization_ = > + picture_->suitableForGpuRasterization(nullptr); > } > } > > > > -- 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.
On 2015/12/18 18:41:31, danakj (behind on reviews) wrote: > On Fri, Dec 18, 2015 at 10:16 AM, <mailto:senorblanco@chromium.org> wrote: > > (This should be covered by DisplayItemListTest, > > IsSuitableForGpuRasterizationWithCachedPicture, but it isn't failing in > > ToT for > > some reason.) > > > > Can you fix the test please? Yep, working on it.
On 2015/12/18 19:21:02, Stephen White wrote: > On 2015/12/18 18:41:31, danakj (behind on reviews) wrote: > > On Fri, Dec 18, 2015 at 10:16 AM, <mailto:senorblanco@chromium.org> wrote: > > > > (This should be covered by DisplayItemListTest, > > > IsSuitableForGpuRasterizationWithCachedPicture, but it isn't failing in > > > ToT for > > > some reason.) > > > > > > > Can you fix the test please? > > Yep, working on it. For some reason, the test calls CreateAndAppendItem(), but Chrome does not. Maybe Chrome is calling CreateFromProto() instead?
danakj@chromium.org changed reviewers: + vmpstr@chromium.org
+vmpstr
I tried adding two more subtests which serialize & deserialize the DisplayItemList, but the second subtest fails, with or without my fix (is_suitable is false in the old list, but true in the new list). Looks like the picture is returning true after deserialization. Is the picture serialized to the proto?
Chrome wouldn't be calling proto related things, since that's exclusively Blimp (and it's not quite finished yet). I think Chrome should be calling CreateAndAppendItem: https://code.google.com/p/chromium/codesearch#chromium/src/cc/blink/web_displ...
On 2015/12/18 20:44:54, vmpstr wrote: > Chrome wouldn't be calling proto related things, since that's exclusively Blimp > (and it's not quite finished yet). Ahh, thanks. I'll ignore the proto stuff, then. > I think Chrome should be calling > CreateAndAppendItem: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/blink/web_displ... I put a printf() in there, and it doesn't get called from Chrome in this case, only from the unit test.
On 2015/12/18 21:00:21, Stephen White wrote: > On 2015/12/18 20:44:54, vmpstr wrote: > > Chrome wouldn't be calling proto related things, since that's exclusively > Blimp > > (and it's not quite finished yet). > > Ahh, thanks. I'll ignore the proto stuff, then. > > > I think Chrome should be calling > > CreateAndAppendItem: > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/blink/web_displ... > > I put a printf() in there, and it doesn't get called from Chrome in this case, > only from the unit test. ("this case" -> Chalkboard (crbug.com/571033)
On 2015/12/18 20:44:54, vmpstr wrote: > Chrome wouldn't be calling proto related things, since that's exclusively Blimp > (and it's not quite finished yet). I think Chrome should be calling > CreateAndAppendItem: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/blink/web_displ... The picture is serialized to the proto using the Skia serialize function. See this: https://code.google.com/p/chromium/codesearch#chromium/src/cc/playback/drawin... Maybe the serialize-function doesn't serialize that? The value seems to be dependent on the number of slow paths in the SkPicture, which is dependent on the SkPicture type. So could very well be something goes awry in the (de)serialization.
After chatting with Vlad, we're guessing it's calling the RasterIntoCanvas() fast path, and not the CreateAndAppendItem() path. Will add a new test to exercise RasterIntoCanvas().
Unit tests added. PTAL. (Note that I'm stack-allocating a DisplayItem. Hope that's ok.)
lgtm, thanks
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538913002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538913002/20001
Message was sent while issue was closed.
Description was changed from ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Restore GPU suitability check in cached_picture case. BUG=571033 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/8d842943f35e5be9cb5607dc948aafd600f018cb Cr-Commit-Position: refs/heads/master@{#366226} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8d842943f35e5be9cb5607dc948aafd600f018cb Cr-Commit-Position: refs/heads/master@{#366226} |