Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2927573003:
cc: Disallow img.decode images from being checker imaged. (Closed)
Descriptioncc: Disallow img.decode images from being checker imaged.
This patch ensures that decodes requested via js decode api don't end
up being checker imaged. This is important since the decode api is one
of the "opt out" paths of checker imaging.
This change also removes the distinction between SYNC_PERMANENT and
SYNC_DECODED_ONCE, since it's unclear we need that.
R=khushalsagar@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2927573003
Cr-Commit-Position: refs/heads/master@{#477830}
Committed: https://chromium.googlesource.com/chromium/src/+/dedd7af3a8d02a8ac218d7ea955a375b1a3473dc
Patch Set 1 #
Total comments: 2
Patch Set 2 : update #Patch Set 3 : update #Patch Set 4 : update #Patch Set 5 : typo #
Total comments: 2
Patch Set 6 : update #Patch Set 7 : update #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== cc: Disallow img.decode images from being checker imaged. This patch ensures that decodes requested via js decode api don't end up being checker imaged. This is important since the decode api is one of the "opt out" paths of checker imaging. R=khushalsagar@chromium.org ========== to ========== cc: Disallow img.decode images from being checker imaged. This patch ensures that decodes requested via js decode api don't end up being checker imaged. This is important since the decode api is one of the "opt out" paths of checker imaging. R=khushalsagar@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Please take a look. Also this patch title is a sentence I never thought I'd type.
https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... cc/tiles/checker_image_tracker.cc:106: image_async_decode_state_[image.stable_id()] = DecodePolicy::SYNC_PERMANENT; I think if the image was already seen by the tracker, then we have to continue checkering it. Because we now we have checker-imaged tiles and we are responsible for invalidating them.
PTAL https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... cc/tiles/checker_image_tracker.cc:106: image_async_decode_state_[image.stable_id()] = DecodePolicy::SYNC_PERMANENT; On 2017/06/07 00:47:21, Khushal wrote: > I think if the image was already seen by the tracker, then we have to continue > checkering it. Because we now we have checker-imaged tiles and we are > responsible for invalidating them. Makes sense. I've added a separate container for tracking this so that it doesn't get muddied with the current state. The new behavior should be that if we already know the state of the image in the decode state, then it will use that. However, when inserting a new state it will consider whether it's disallowed or not. I've also changed the clear function a little bit so that instead of marking images as async if they were sync decoded with pending invalidations, instead they just get deleted from the state. This would again make then async when we check if we should checker them, unless of course they were disallowed meanwhile. I think this is fine, since we clear the pending invalidation set anyway, so we know there are no tiles right now. Is that correct? In other words, if we are "soft" clearing the tracker, then that means we're not visible which means we've evicted all tiles. If that wouldn't be the case, then we can't clear the images pending invalidation, afaict.
On 2017/06/07 17:05:36, vmpstr wrote: > PTAL > > https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... > File cc/tiles/checker_image_tracker.cc (right): > > https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... > cc/tiles/checker_image_tracker.cc:106: > image_async_decode_state_[image.stable_id()] = DecodePolicy::SYNC_PERMANENT; > On 2017/06/07 00:47:21, Khushal wrote: > > I think if the image was already seen by the tracker, then we have to continue > > checkering it. Because we now we have checker-imaged tiles and we are > > responsible for invalidating them. > > Makes sense. I've added a separate container for tracking this so that it > doesn't get muddied with the current state. The new behavior should be that if > we already know the state of the image in the decode state, then it will use > that. However, when inserting a new state it will consider whether it's > disallowed or not. > > I've also changed the clear function a little bit so that instead of marking > images as async if they were sync decoded with pending invalidations, instead > they just get deleted from the state. This would again make then async when we > check if we should checker them, unless of course they were disallowed > meanwhile. I think this is fine, since we clear the pending invalidation set > anyway, so we know there are no tiles right now. Is that correct? Yeah, we are assuming there are no tiles so we will see calls to ShouldCheckerImage again and then re-decode. Its because we know the image cache is also cleared when going invisible, so we gotta lock it again. What you have is correct, but I'm not sure if having yet another state to track is worth it to handle this particular case. Essentially what we are saying is that in case the image was on the page, we checkered it, decoded it but became invisible before we could show it. Then don't checker it again in case the decode API had also asked for it. The behaviour is already so undefined with whether using the decode API after displaying the image to the user will checker it or not. I think we should just assume that you have to use the decode API before you put the image on the page. Otherwise, what's the point? You caused jank by forcing us to sync decode it anyway. > > In other words, if we are "soft" clearing the tracker, then that means we're not > visible which means we've evicted all tiles. If that wouldn't be the case, then > we can't clear the images pending invalidation, afaict.
On 2017/06/07 18:08:52, Khushal wrote: > On 2017/06/07 17:05:36, vmpstr wrote: > > PTAL > > > > > https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... > > File cc/tiles/checker_image_tracker.cc (right): > > > > > https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... > > cc/tiles/checker_image_tracker.cc:106: > > image_async_decode_state_[image.stable_id()] = DecodePolicy::SYNC_PERMANENT; > > On 2017/06/07 00:47:21, Khushal wrote: > > > I think if the image was already seen by the tracker, then we have to > continue > > > checkering it. Because we now we have checker-imaged tiles and we are > > > responsible for invalidating them. > > > > Makes sense. I've added a separate container for tracking this so that it > > doesn't get muddied with the current state. The new behavior should be that if > > we already know the state of the image in the decode state, then it will use > > that. However, when inserting a new state it will consider whether it's > > disallowed or not. > > > > I've also changed the clear function a little bit so that instead of marking > > images as async if they were sync decoded with pending invalidations, instead > > they just get deleted from the state. This would again make then async when we > > check if we should checker them, unless of course they were disallowed > > meanwhile. I think this is fine, since we clear the pending invalidation set > > anyway, so we know there are no tiles right now. Is that correct? > > Yeah, we are assuming there are no tiles so we will see calls to > ShouldCheckerImage again and then re-decode. Its because we know the image cache > is also cleared when going invisible, so we gotta lock it again. > > What you have is correct, but I'm not sure if having yet another state to track > is worth it to handle this particular case. Essentially what we are saying is > that in case the image was on the page, we checkered it, decoded it but became > invisible before we could show it. Then don't checker it again in case the > decode API had also asked for it. The behaviour is already so undefined with > whether using the decode API after displaying the image to the user will checker > it or not. I think we should just assume that you have to use the decode API > before you put the image on the page. Otherwise, what's the point? You caused > jank by forcing us to sync decode it anyway. I kind of prefer having a separate state here, since it's fundamentally unrelated to checker imaging in general. It's just a knob to force some images to ignore other things we do here. This makes it easier to change the state to store tasks or draw images or whatever instead of simply a flag, if we decide to do that in the future. I'm not sure what trying to have a single state is buying us here, since it becomes easy to forget that we have some things disabled, and thus have to introduce a bunch of ifs throughout the code. > > > > > In other words, if we are "soft" clearing the tracker, then that means we're > not > > visible which means we've evicted all tiles. If that wouldn't be the case, > then > > we can't clear the images pending invalidation, afaict.
On 2017/06/07 18:19:01, vmpstr wrote: > On 2017/06/07 18:08:52, Khushal wrote: > > On 2017/06/07 17:05:36, vmpstr wrote: > > > PTAL > > > > > > > > > https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... > > > File cc/tiles/checker_image_tracker.cc (right): > > > > > > > > > https://codereview.chromium.org/2927573003/diff/1/cc/tiles/checker_image_trac... > > > cc/tiles/checker_image_tracker.cc:106: > > > image_async_decode_state_[image.stable_id()] = DecodePolicy::SYNC_PERMANENT; > > > On 2017/06/07 00:47:21, Khushal wrote: > > > > I think if the image was already seen by the tracker, then we have to > > continue > > > > checkering it. Because we now we have checker-imaged tiles and we are > > > > responsible for invalidating them. > > > > > > Makes sense. I've added a separate container for tracking this so that it > > > doesn't get muddied with the current state. The new behavior should be that > if > > > we already know the state of the image in the decode state, then it will use > > > that. However, when inserting a new state it will consider whether it's > > > disallowed or not. > > > > > > I've also changed the clear function a little bit so that instead of marking > > > images as async if they were sync decoded with pending invalidations, > instead > > > they just get deleted from the state. This would again make then async when > we > > > check if we should checker them, unless of course they were disallowed > > > meanwhile. I think this is fine, since we clear the pending invalidation set > > > anyway, so we know there are no tiles right now. Is that correct? > > > > Yeah, we are assuming there are no tiles so we will see calls to > > ShouldCheckerImage again and then re-decode. Its because we know the image > cache > > is also cleared when going invisible, so we gotta lock it again. > > > > What you have is correct, but I'm not sure if having yet another state to > track > > is worth it to handle this particular case. Essentially what we are saying is > > that in case the image was on the page, we checkered it, decoded it but became > > invisible before we could show it. Then don't checker it again in case the > > decode API had also asked for it. The behaviour is already so undefined with > > whether using the decode API after displaying the image to the user will > checker > > it or not. I think we should just assume that you have to use the decode API > > before you put the image on the page. Otherwise, what's the point? You caused > > jank by forcing us to sync decode it anyway. > > I kind of prefer having a separate state here, since it's fundamentally > unrelated to checker imaging in general. It's just a knob to force some images > to ignore other things we do here. This makes it easier to change the state to > store tasks or draw images or whatever instead of simply a flag, if we decide to > do that in the future. I'm not sure what trying to have a single state is buying > us here, since it becomes easy to forget that we have some things disabled, and > thus have to introduce a bunch of ifs throughout the code. I'm all for adding more state to track if its necessary. I already made it a struct in https://codereview.chromium.org/2928433003/ to get DrawImages going. What I'm suggesting is to just set the DecodePolicy to SYNC_PERMANENT if it was not already being tracked. If it is already being tracked, then we can live with it whatever decision we made. If image.decode is used before showing the image to the user, then it will make sure that the image is not checkered. > > > > > > > > > In other words, if we are "soft" clearing the tracker, then that means we're > > not > > > visible which means we've evicted all tiles. If that wouldn't be the case, > > then > > > we can't clear the images pending invalidation, afaict.
Description was changed from ========== cc: Disallow img.decode images from being checker imaged. This patch ensures that decodes requested via js decode api don't end up being checker imaged. This is important since the decode api is one of the "opt out" paths of checker imaging. R=khushalsagar@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Disallow img.decode images from being checker imaged. This patch ensures that decodes requested via js decode api don't end up being checker imaged. This is important since the decode api is one of the "opt out" paths of checker imaging. This change also removes the distinction between SYNC_PERMANENT and SYNC_DECODED_ONCE, since it's unclear we need that. R=khushalsagar@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Per offline discussion, I've removed the distinction between SYNC_PERMANENT and SYNC_DECODED_ONCE, which makes it more reasonable to use a single enum to track this state. PTAL
lgtm. Thanks. https://codereview.chromium.org/2927573003/diff/70001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2927573003/diff/70001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:64: void DisallowCheckeringForImage(const PaintImage& image); Could you leave a comment saying that it would do it if they were not already checkered?
https://codereview.chromium.org/2927573003/diff/70001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2927573003/diff/70001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:64: void DisallowCheckeringForImage(const PaintImage& image); On 2017/06/07 20:38:06, Khushal wrote: > Could you leave a comment saying that it would do it if they were not already > checkered? Done.
The CQ bit was checked by vmpstr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khushalsagar@chromium.org Link to the patchset: https://codereview.chromium.org/2927573003/#ps110001 (title: "update")
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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 110001, "attempt_start_ts": 1496877067448430,
"parent_rev": "8cae4d389092473a097baf547b1fecfe2259e748", "commit_rev":
"f0b60ce3ded7761fc36c1e9a988567bcda6492a3"}
CQ is committing da patch.
Bot data: {"patchset_id": 110001, "attempt_start_ts": 1496877067448430,
"parent_rev": "c43b3cc74115cf173788d8856ce2866bab221a54", "commit_rev":
"dedd7af3a8d02a8ac218d7ea955a375b1a3473dc"}
Message was sent while issue was closed.
Description was changed from ========== cc: Disallow img.decode images from being checker imaged. This patch ensures that decodes requested via js decode api don't end up being checker imaged. This is important since the decode api is one of the "opt out" paths of checker imaging. This change also removes the distinction between SYNC_PERMANENT and SYNC_DECODED_ONCE, since it's unclear we need that. R=khushalsagar@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Disallow img.decode images from being checker imaged. This patch ensures that decodes requested via js decode api don't end up being checker imaged. This is important since the decode api is one of the "opt out" paths of checker imaging. This change also removes the distinction between SYNC_PERMANENT and SYNC_DECODED_ONCE, since it's unclear we need that. R=khushalsagar@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2927573003 Cr-Commit-Position: refs/heads/master@{#477830} Committed: https://chromium.googlesource.com/chromium/src/+/dedd7af3a8d02a8ac218d7ea955a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/chromium/src/+/dedd7af3a8d02a8ac218d7ea955a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
