ImageDecodeBench packeted runs should resume decoding where it left off

third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp

Index: third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
diff --git a/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp b/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
index 4ef02ef74e1dbe60c1868871e3dfae3e2aab0fa7..9fc57c794207db0f0656bb22664ae52753ff4d17 100644
--- a/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
+++ b/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
@@ -226,6 +226,7 @@ bool DecodeImageData(SharedBuffer* data,
 
   RefPtr<SharedBuffer> packet_data = SharedBuffer::Create();
   size_t position = 0;
+  size_t next_frame_to_decode = 0;
   while (true) {
     const char* packet;
     size_t length = data->GetSomeData(packet, position);
@@ -235,12 +236,14 @@ bool DecodeImageData(SharedBuffer* data,
     position += length;
 
     bool all_data_received = position == data->size();
-    decoder->SetData(packet_data.Get(), all_data_received);
-
-    int frame_count = decoder->FrameCount();
-    for (int i = 0; i < frame_count; ++i) {
-      if (!decoder->FrameBufferAtIndex(i))
+    size_t frame_count = decoder->FrameCount();
+    for (; next_frame_to_decode < frame_count; ++next_frame_to_decode) {
+      decoder->SetData(packet_data.Get(), all_data_received);
+      ImageFrame* frame = decoder->FrameBufferAtIndex(next_frame_to_decode);
+      if (frame->GetStatus() != ImageFrame::kFrameComplete)

[scroggo_chromium, 2017/05/15 12:48:28]

I think this belongs at the end of the loop. Notice how
ImageFrameGenerator::Decode calls SetData(null) and ClearCacheExceptFrame
regardless of the FrameStatus.

[cblume, 2017/05/15 20:55:09]

This is effectively done now that the SetData reset/clear and
ClearCacheExceptFrame have been removed.

         break;
+      decoder->SetData(PassRefPtr<SegmentReader>(nullptr), false);
+      decoder->ClearCacheExceptFrame(next_frame_to_decode);

[scroggo_chromium, 2017/05/15 12:48:28]

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.

[cblume, 2017/05/15 17:39:11]

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.

[scroggo_chromium, 2017/05/15 18:09:22]

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.

     }
 
     if (all_data_received || decoder->Failed())