Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880533002:
ImageDecodeBench packeted runs should resume decoding where it left off (Closed)
DescriptionResume ImageDecodeBench packeted runs where it left off
ImageDecodeBench has a mode where it fills the buffer with data of a
given packet size. It then decodes as much as it can for each data
packet received. I believe the point of this mode is to test a more
real-world decoding scenario (using network-sized packets).
However, ImageDecodeBench fetches already-decoded frames from animated
images when a new packet arrives, rather than continuing to decode the
next frame. For example, if we decoded the first 3 frames with packet
1, upon packet 2 coming in, we fetch those first 3 frames again before
decoding frame 4. This does not cause an expensive decode, but still
deviates from the real-world decoding scenario.
In this change, make ImageDecodeBench resume where it left off when in
packet timing mode.
BUG=721260
Review-Url: https://codereview.chromium.org/2880533002
Cr-Commit-Position: refs/heads/master@{#473645}
Committed: https://chromium.googlesource.com/chromium/src/+/40bdf8dbdc4089b110e76ea1d063a9b92f615301
Patch Set 1 #
Total comments: 2
Patch Set 2 : Making the packeted test more closely mimic cache clearing and resetting data #
Total comments: 5
Patch Set 3 : Remove ClearCacheExceptFrame & calls to SetData to reset and clear #
Dependent Patchsets: Messages
Total messages: 49 (26 generated)
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/v2/patch-status/codereview.chromium.or...
cblume@chromium.org changed reviewers: + noel@chromium.org, pkasting@chromium.org, scroggo@chromium.org
While cleaning up timing I noticed this behavior probably isn't what we wanted. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scroggo@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/2880533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:242: for (int i = next_frame_to_decode; i < frame_count; ++i) { FWIW, starting from frame 0 doesn't re-decode those first three frames (one exception - when purging aggressively, they *will* need to be re-decoded). We don't clear the cache, so they should still be there and complete, so FrameBufferAtIndex will just return the complete frames. But you're right, no need to call FrameBufferAtIndex for those frames (especially for the exception of purging aggressively). I don't think this change goes far enough, though. FrameCount() should only return frames that we at least have a piece of, and FrameBufferAtIndex only returns null if i >= FrameCount(). So I don't think we'll ever hit this case (meaning that next_frame_to_decode will remain 0). Instead, I think you want something like the following: for (; next_frame_to_decode < frame_count; ++next_frame_to_decode) { auto status = decoder->FrameBufferAtIndex(next_frame_to_decode)->GetStatus(); if (status != ImageFrame::FrameComplete) { break; }
Description was changed from ========== ImageDecodeBench packeted runs should resume decoding where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench restarts animated images for each packet. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we decode those first 3 frames again before decoding frame 4. This deviates from the real-world decoding scenario. So ImageDecodeBench should resume where it left off for packeted timing mode. BUG=721260 ========== to ========== ImageDecodeBench packeted runs should resume decoding where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench restarts animated images for each packet. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we decode those first 3 frames again before decoding frame 4. This deviates from the real-world decoding scenario. So ImageDecodeBench should resume where it left off for packeted timing mode. BUG=721260 ==========
pkasting@chromium.org changed reviewers: - pkasting@chromium.org
Removing myself since it looks like you've got other capable folks reviewing here.
https://codereview.chromium.org/2880533002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880533002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:242: for (int i = next_frame_to_decode; i < frame_count; ++i) { On 2017/05/11 13:56:23, scroggo wrote: > FWIW, starting from frame 0 doesn't re-decode those first three frames (one > exception - when purging aggressively, they *will* need to be re-decoded). We > don't clear the cache, so they should still be there and complete, so > FrameBufferAtIndex will just return the complete frames. > > But you're right, no need to call FrameBufferAtIndex for those frames > (especially for the exception of purging aggressively). > > I don't think this change goes far enough, though. FrameCount() should only > return frames that we at least have a piece of, and FrameBufferAtIndex only > returns null if i >= FrameCount(). So I don't think we'll ever hit this case > (meaning that next_frame_to_decode will remain 0). Instead, I think you want > something like the following: > > for (; next_frame_to_decode < frame_count; ++next_frame_to_decode) { > auto status = > decoder->FrameBufferAtIndex(next_frame_to_decode)->GetStatus(); > if (status != ImageFrame::FrameComplete) { > break; > } Ah. I agree. Since I believe this test is aimed at replicating real-world behavior, I might want to add the purging as well. I don't want to time it (once I fix the timing). But the act of purging could impact instruction and data caches. What do you think? Also Leon, do you prefer I add your google address, chromium address, or both? I added your chromium address because you've given LGTMs from it before. But maybe you want both?
On 2017/05/11 15:45:31, cblume wrote: > https://codereview.chromium.org/2880533002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): > > https://codereview.chromium.org/2880533002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:242: for (int i > = next_frame_to_decode; i < frame_count; ++i) { > On 2017/05/11 13:56:23, scroggo wrote: > > FWIW, starting from frame 0 doesn't re-decode those first three frames (one > > exception - when purging aggressively, they *will* need to be re-decoded). We > > don't clear the cache, so they should still be there and complete, so > > FrameBufferAtIndex will just return the complete frames. > > > > But you're right, no need to call FrameBufferAtIndex for those frames > > (especially for the exception of purging aggressively). > > > > I don't think this change goes far enough, though. FrameCount() should only > > return frames that we at least have a piece of, and FrameBufferAtIndex only > > returns null if i >= FrameCount(). So I don't think we'll ever hit this case > > (meaning that next_frame_to_decode will remain 0). Instead, I think you want > > something like the following: > > > > for (; next_frame_to_decode < frame_count; ++next_frame_to_decode) { > > auto status = > > decoder->FrameBufferAtIndex(next_frame_to_decode)->GetStatus(); > > if (status != ImageFrame::FrameComplete) { > > break; > > } > > Ah. I agree. > > Since I believe this test is aimed at replicating real-world behavior, I might > want to add the purging as well. I don't want to time it (once I fix the > timing). But the act of purging could impact instruction and data caches. What > do you think? I'm not aware what impact it would have on instruction or data caches, but if you clear the cache and then try to continue decoding, when you clear the cache would be its own variable. > > Also Leon, do you prefer I add your google address, chromium address, or both? I > added your chromium address because you've given LGTMs from it before. But maybe > you want both? Haha, sorry. Generally I want you to use my Chromium address (and I think that's the only one whose approval matters), but occasionally I'll accidentally open in the wrong window and use my Google address. If you add either one, though, I'll get an email, so it will work either way.
On 2017/05/11 16:55:48, scroggo_chromium wrote: > when you clear the cache would be its own variable. You're right about that. I think I would try to recreate current behavior (IE place it where we currently purge, which is just after the decode [1]). The whole point of this packeted thing is to mirror what happens in Chrome, I believe. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
FYI: You seem to have accidentally landed these changes in crrev.com/2857753002 https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:243: if (frame->GetStatus() != ImageFrame::kFrameComplete) I think this belongs at the end of the loop. Notice how ImageFrameGenerator::Decode calls SetData(null) and ClearCacheExceptFrame regardless of the FrameStatus. https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:246: decoder->ClearCacheExceptFrame(next_frame_to_decode); I'm ambivalent about adding ClearCacheExceptFrame and removing/resetting the data. It is more in line with how we do things right now, although if we change anything it seems like it would be easy to get the two out of sync. At the very least, it deserves a comment, explaining that this replicates the behavior in ImageFrameGenerator::Decode, and probably a comment at the top where it explains what this benchmark does.
> However, ImageDecodeBench restarts animated images for each packet. For > example, if we could decode the first 3 frames with packet 1, upon > packet 2 coming in we decode those first 3 frames again before decoding > frame 4. This deviates from the real-world decoding scenario. Would you please update the commit message, too? We're not actually decoding those first three frames again, IIUC. We're calling FrameBufferAtIndex, but if they are already decoded, we won't be doing much work (we're just checking their status). Also, if we include the bit that clears the cache, that should be in the commit message as well.
https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:246: decoder->ClearCacheExceptFrame(next_frame_to_decode); On 2017/05/15 12:48:28, scroggo_chromium wrote: > I'm ambivalent about adding ClearCacheExceptFrame and removing/resetting the > data. It is more in line with how we do things right now, although if we change > anything it seems like it would be easy to get the two out of sync. > > At the very least, it deserves a comment, explaining that this replicates the > behavior in ImageFrameGenerator::Decode, and probably a comment at the top where > it explains what this benchmark does. I agree with you. This runs the risk of becoming out-of-sync. A better option is an integration test into Chromium, which we already have. Maybe the real solution here is to just not have a packeted decode test? I can't think of any reason to have it other than replicating that integration test.
https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:246: decoder->ClearCacheExceptFrame(next_frame_to_decode); On 2017/05/15 17:39:11, cblume wrote: > On 2017/05/15 12:48:28, scroggo_chromium wrote: > > I'm ambivalent about adding ClearCacheExceptFrame and removing/resetting the > > data. It is more in line with how we do things right now, although if we > change > > anything it seems like it would be easy to get the two out of sync. > > > > At the very least, it deserves a comment, explaining that this replicates the > > behavior in ImageFrameGenerator::Decode, and probably a comment at the top > where > > it explains what this benchmark does. > > I agree with you. This runs the risk of becoming out-of-sync. A better option is > an integration test into Chromium, which we already have. > > Maybe the real solution here is to just not have a packeted decode test? > > I can't think of any reason to have it other than replicating that integration > test. Where is the integration test? FWIW, I added the packet mode because I was working on changes that I expected might have an impact on decoding in packets that was different than having all of the data available up front. If that is covered by another test, then perhaps you're right.
On 2017/05/15 18:09:22, scroggo_chromium wrote: > https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): > > https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:246: > decoder->ClearCacheExceptFrame(next_frame_to_decode); > On 2017/05/15 17:39:11, cblume wrote: > > On 2017/05/15 12:48:28, scroggo_chromium wrote: > > > I'm ambivalent about adding ClearCacheExceptFrame and removing/resetting the > > > data. It is more in line with how we do things right now, although if we > > change > > > anything it seems like it would be easy to get the two out of sync. > > > > > > At the very least, it deserves a comment, explaining that this replicates > the > > > behavior in ImageFrameGenerator::Decode, and probably a comment at the top > > where > > > it explains what this benchmark does. > > > > I agree with you. This runs the risk of becoming out-of-sync. A better option > is > > an integration test into Chromium, which we already have. > > > > Maybe the real solution here is to just not have a packeted decode test? > > > > I can't think of any reason to have it other than replicating that integration > > test. > > Where is the integration test? > > FWIW, I added the packet mode because I was working on changes that I expected > might have an impact on decoding in packets that was different than having all > of the data available up front. If that is covered by another test, then perhaps > you're right. The integration test is in telemetry, which gets automatically run by the bots and notifies us if things change. You can also run it locally. https://cs.chromium.org/chromium/src/tools/perf/page_sets/image_decoding_meas... It doesn't allow for the fine-tuning of packet size, though. So they aren't exactly the same. But if the point is to mimic Chromium behavior then maybe we don't need that since there are so many other variables in play that are unaccounted for.
On 2017/05/15 18:59:42, cblume wrote: > On 2017/05/15 18:09:22, scroggo_chromium wrote: > > > https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): > > > > > https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:246: > > decoder->ClearCacheExceptFrame(next_frame_to_decode); > > On 2017/05/15 17:39:11, cblume wrote: > > > On 2017/05/15 12:48:28, scroggo_chromium wrote: > > > > I'm ambivalent about adding ClearCacheExceptFrame and removing/resetting > the > > > > data. It is more in line with how we do things right now, although if we > > > change > > > > anything it seems like it would be easy to get the two out of sync. > > > > > > > > At the very least, it deserves a comment, explaining that this replicates > > the > > > > behavior in ImageFrameGenerator::Decode, and probably a comment at the top > > > where > > > > it explains what this benchmark does. > > > > > > I agree with you. This runs the risk of becoming out-of-sync. A better > option > > is > > > an integration test into Chromium, which we already have. > > > > > > Maybe the real solution here is to just not have a packeted decode test? > > > > > > I can't think of any reason to have it other than replicating that > integration > > > test. > > > > Where is the integration test? > > > > FWIW, I added the packet mode because I was working on changes that I expected > > might have an impact on decoding in packets that was different than having all > > of the data available up front. If that is covered by another test, then > perhaps > > you're right. > > The integration test is in telemetry, which gets automatically run by the bots > and notifies us if things change. You can also run it locally. > https://cs.chromium.org/chromium/src/tools/perf/page_sets/image_decoding_meas... > > It doesn't allow for the fine-tuning of packet size, though. So they aren't > exactly the same. > But if the point is to mimic Chromium behavior then maybe we don't need that > since there are so many other variables in play that are unaccounted for. I see. Some downsides to the integration test, relative to the packet-size test, for the case for which the latter was written: - it only tests a couple of images - If I understand the integration test, it will have the entire image in memory immediately, so it won't test downloading in packets at all. packet-size is testing a bad case, where we have to download more data on a bad connection I had considered rewriting the test to use a DeferredImageDecoder (in fact, I wrote up a CL that does that at crrev.com/1821043004), which would handle the caching as you're trying to do here, but just using the packets manually seemed like a good enough approach, since my goal at the time was to see the impact of copying the input data repeatedly. This was independent of caching or other factors. All that said, I'd be fine if you want to remove it. It's only useful if it's being used, and it's currently not automated. I would only be inclined to use it in the future if I think a change would similarly be affected by the packet size. And since there's already a test that is automated (I believe the integration test is?), then perhaps that's a better place to start from.
+Ned IIRC, telemetry replays a live capture. So rather than loading from disc and having the entire image immediately available in memory, it will stream in data packed by packet. But I might be recalling that incorrectly. If that is how it behaves, that also means the downside of it is you can't tune over packet size. So we couldn't use the existing thing as a starting point. But maybe we can setup an integration test that does automate this. For now, let's leave this function in place and I'll remove the ClearCacheExceptFrame and removing/resetting the data. That way we can deal with possibly automating this in a separate patch.
On 2017/05/15 20:19:37, cblume wrote: > +Ned > > IIRC, telemetry replays a live capture. So rather than loading from disc and > having the entire image immediately available in memory, it will stream in data > packed by packet. > > But I might be recalling that incorrectly. > > If that is how it behaves, that also means the downside of it is you can't tune > over packet size. So we couldn't use the existing thing as a starting point. > > But maybe we can setup an integration test that does automate this. > > > For now, let's leave this function in place and I'll remove the > ClearCacheExceptFrame and removing/resetting the > data. That way we can deal with possibly automating this in a separate patch. sgtm
On 2017/05/15 20:19:37, cblume wrote: > +Ned > > IIRC, telemetry replays a live capture. So rather than loading from disc and > having the entire image immediately available in memory, it will stream in data > packed by packet. Right. Even for static http test cases, Telemetry also send data by packet here (https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...). Though, I suggest running the teleemtry benchmark with more detailed tracing enabled to figure out which is the case here. > > But I might be recalling that incorrectly. > > If that is how it behaves, that also means the downside of it is you can't tune > over packet size. So we couldn't use the existing thing as a starting point. > > But maybe we can setup an integration test that does automate this. > > > For now, let's leave this function in place and I'll remove the > ClearCacheExceptFrame and removing/resetting the > data. That way we can deal with possibly automating this in a separate patch.
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:243: if (frame->GetStatus() != ImageFrame::kFrameComplete) On 2017/05/15 12:48:28, scroggo_chromium wrote: > I think this belongs at the end of the loop. Notice how > ImageFrameGenerator::Decode calls SetData(null) and ClearCacheExceptFrame > regardless of the FrameStatus. This is effectively done now that the SetData reset/clear and ClearCacheExceptFrame have been removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please update the commit message as I suggested earlier in comment 20: > However, ImageDecodeBench restarts animated images for each packet. For > example, if we could decode the first 3 frames with packet 1, upon > packet 2 coming in we decode those first 3 frames again before decoding > frame 4. This deviates from the real-world decoding scenario. Would you please update the commit message, too? We're not actually decoding those first three frames again, IIUC. We're calling FrameBufferAtIndex, but if they are already decoded, we won't be doing much work (we're just checking their status).
Description was changed from ========== ImageDecodeBench packeted runs should resume decoding where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench restarts animated images for each packet. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we decode those first 3 frames again before decoding frame 4. This deviates from the real-world decoding scenario. So ImageDecodeBench should resume where it left off for packeted timing mode. BUG=721260 ========== to ========== ImageDecodeBench packeted runs should resume decoding where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. So ImageDecodeBench should resume where it left off for packeted timing mode. BUG=721260 ==========
Description was changed from ========== ImageDecodeBench packeted runs should resume decoding where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. So ImageDecodeBench should resume where it left off for packeted timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. Make ImageDecodeBench resume where it left off for packeted timing mode. BUG=721260 ==========
On 2017/05/16 13:34:35, scroggo_chromium wrote: > Please update the commit message as I suggested earlier in comment 20: > > > However, ImageDecodeBench restarts animated images for each packet. For > > example, if we could decode the first 3 frames with packet 1, upon > > packet 2 coming in we decode those first 3 frames again before decoding > > frame 4. This deviates from the real-world decoding scenario. > > Would you please update the commit message, too? We're not actually decoding > those first three frames again, IIUC. We're calling FrameBufferAtIndex, but if > they are already decoded, we won't be doing much work (we're just checking their > status). Done. Also, your note about imperative mode makes a lot of sense to me. I've applied that here and I'll be more careful about that going forward.
On 2017/05/19 16:44:44, cblume wrote: > On 2017/05/16 13:34:35, scroggo_chromium wrote: > > Please update the commit message as I suggested earlier in comment 20: > > > > > However, ImageDecodeBench restarts animated images for each packet. For > > > example, if we could decode the first 3 frames with packet 1, upon > > > packet 2 coming in we decode those first 3 frames again before decoding > > > frame 4. This deviates from the real-world decoding scenario. > > > > Would you please update the commit message, too? We're not actually decoding > > those first three frames again, IIUC. We're calling FrameBufferAtIndex, but if > > they are already decoded, we won't be doing much work (we're just checking > their > > status). > > Done. > Also, your note about imperative mode makes a lot of sense to me. I've applied > that here and I'll be more careful about that going forward. Thanks! LGTM. You'll still need an OWNERS approval, though.
On 2017/05/19 16:57:03, scroggo_chromium wrote: > On 2017/05/19 16:44:44, cblume wrote: > > On 2017/05/16 13:34:35, scroggo_chromium wrote: > > > Please update the commit message as I suggested earlier in comment 20: > > > > > > > However, ImageDecodeBench restarts animated images for each packet. For > > > > example, if we could decode the first 3 frames with packet 1, upon > > > > packet 2 coming in we decode those first 3 frames again before decoding > > > > frame 4. This deviates from the real-world decoding scenario. > > > > > > Would you please update the commit message, too? We're not actually decoding > > > those first three frames again, IIUC. We're calling FrameBufferAtIndex, but > if > > > they are already decoded, we won't be doing much work (we're just checking > > their > > > status). > > > > Done. > > Also, your note about imperative mode makes a lot of sense to me. I've applied > > that here and I'll be more careful about that going forward. > > Thanks! LGTM. You'll still need an OWNERS approval, though. Yep. Noel@ is an owner of platform/ (there is no OWNERS file for platform/testing/). Noel, when you get a chance PTAL.
Description was changed from ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario. However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. Make ImageDecodeBench resume where it left off for packeted timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. (I believe the point of this mode is to test a more real-world decoding scenario with network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. Make ImageDecodeBench resume where it left off for packeted timing mode. BUG=721260 ==========
Description was changed from ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. (I believe the point of this mode is to test a more real-world decoding scenario with network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. Make ImageDecodeBench resume where it left off for packeted timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. Make ImageDecodeBench resume where it left off for packeted timing mode. BUG=721260 ==========
Description was changed from ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given packet size. It then decodes as much as it can for each virtual packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we could decode the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. Make ImageDecodeBench resume where it left off for packeted timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given data packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packetized timing mode. BUG=721260 ==========
Description was changed from ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with a given data packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packetized timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with data of a given packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packetized timing mode. BUG=721260 ==========
On 2017/05/19 17:38:31, cblume wrote: > Yep. Noel@ is an owner of platform/ (there is no OWNERS file for > platform/testing/). > Noel, when you get a chance PTAL. Looked over the discussion / code, and this patch LGTM.
Description was changed from ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with data of a given packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packetized timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with data of a given packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packet timing mode. BUG=721260 ==========
The CQ bit was checked by cblume@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": 40001, "attempt_start_ts": 1495474938632160,
"parent_rev": "fabbf757ba1f0fe03a5671eb0bb4936efbb4a890", "commit_rev":
"40bdf8dbdc4089b110e76ea1d063a9b92f615301"}
Message was sent while issue was closed.
Description was changed from ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with data of a given packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packet timing mode. BUG=721260 ========== to ========== Resume ImageDecodeBench packeted runs where it left off ImageDecodeBench has a mode where it fills the buffer with data of a given packet size. It then decodes as much as it can for each data packet received. I believe the point of this mode is to test a more real-world decoding scenario (using network-sized packets). However, ImageDecodeBench fetches already-decoded frames from animated images when a new packet arrives, rather than continuing to decode the next frame. For example, if we decoded the first 3 frames with packet 1, upon packet 2 coming in, we fetch those first 3 frames again before decoding frame 4. This does not cause an expensive decode, but still deviates from the real-world decoding scenario. In this change, make ImageDecodeBench resume where it left off when in packet timing mode. BUG=721260 Review-Url: https://codereview.chromium.org/2880533002 Cr-Commit-Position: refs/heads/master@{#473645} Committed: https://chromium.googlesource.com/chromium/src/+/40bdf8dbdc4089b110e76ea1d063... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/40bdf8dbdc4089b110e76ea1d063... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
