Don't wait endlessly for flushing the batched queries sent to the GPU driver during H/W decode

content/common/gpu/media/dxva_video_decode_accelerator.cc

Index: content/common/gpu/media/dxva_video_decode_accelerator.cc
===================================================================
--- content/common/gpu/media/dxva_video_decode_accelerator.cc	(revision 167469)
+++ content/common/gpu/media/dxva_video_decode_accelerator.cc	(working copy)
@@ -73,6 +73,10 @@
                                log << ", HRESULT: 0x" << std::hex << result, \
                                error_code, ret);
 
+// Maximum number of iterations we allow before aborting the attempt to flush
+// the batched queries to the driver.

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

Why is it safe to give up?  What is the cost to correctness?
Just torn frames or something else?
Should we be UMA-reporting the event and/or the # of iterations it takes to get
through the flush?

Is there a blocking version of the flush you could use instead?

Is it possible that the Sleep(1) isn't enough of a yield for the driver in
question, and that's the problem?

[ananta, 2012/11/13 23:17:22]

Cost to correctness would be torn frames in the worst case. On my machine it
seems to work correctly even without the flush code. We are doing this to
prevent a race condition. 

There is no blocking version of the flush call.

As regards the Sleep(1), needs more investigation. will do that in a separate
change. This bug seems very bad. would like this to be fixed ASAP.

Will also do the UMA reporting in a subsequent change.

[apatrick_chromium, 2012/11/14 01:27:49]

I expect it will be torn frames or similar. It won't crash. We could test that
by replacing the loop with a sleep.

+#define MAX_ITERATIONS_FOR_D3D_FLUSH  10

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

prefer enum to #define
enum { kMaxiterationsForD3dFlush = 10 };

+
 static IMFSample* CreateEmptySample() {
   base::win::ScopedComPtr<IMFSample> sample;
   HRESULT hr = MFCreateSample(sample.Receive());
@@ -135,12 +139,12 @@
   CHECK_GE(static_cast<int>(max_length), size);
   memcpy(destination, stream, size);
 
+  hr = buffer->SetCurrentLength(size);

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

what's this move about?  unmentioned in CL description.

[ananta, 2012/11/13 23:17:22]

Sorry. needless change crept in. was debugging something else.

+  RETURN_ON_HR_FAILURE(hr, "Failed to set buffer length", NULL);
+
   hr = buffer->Unlock();
   RETURN_ON_HR_FAILURE(hr, "Failed to unlock buffer", NULL);
 
-  hr = buffer->SetCurrentLength(size);
-  RETURN_ON_HR_FAILURE(hr, "Failed to set buffer length", NULL);
-
   return sample.Detach();
 }
 
@@ -396,11 +400,22 @@
     // the texture. Flush it once here though.
     hr = query_->Issue(D3DISSUE_END);
     RETURN_ON_HR_FAILURE(hr, "Failed to issue END", false);
+
+    // The loop below attempts to flush the batched queries to the driver. We
+    // don't want to loop endlessly waiting for the driver to return success.

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

...because we've seen driver bugs?

+    // The current workaround is to allow a max of MAX_ITERATIONS_FOR_D3D_FLUSH
+    // iterations.
+    int iterations = 0;
     do {
+      if (iterations > MAX_ITERATIONS_FOR_D3D_FLUSH)

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

putting this test in the while condition would obviate the need for the if/break

[ananta, 2012/11/13 23:17:22]

Done.

+        break;
       hr = query_->GetData(NULL, 0, D3DGETDATA_FLUSH);
-      if (hr == S_FALSE)
+      if (hr == S_FALSE) {
         Sleep(1);  // Poor-man's Yield().
+        ++iterations;
+      }
     } while (hr == S_FALSE);

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

l.408-417 could be rewritten as:

int iterations = 0;
while (query_->GetData(NULL, 0, D3DGETDATA_FLUSH) == S_FALSE) &&
       ++iterations < MAX_ITERATIONS_FOR_D3D_FLUSH) {
  Sleep(1);  // Poor-man's Yield().
}

Not only would it be >50% shorter, it would also not sleep right before giving
up on iterations being too large.

[ananta, 2012/11/13 23:17:22]

Done.

+
     eglBindTexImage(
         static_cast<EGLDisplay*>(eglGetDisplay(EGL_DEFAULT_DISPLAY)),
         decoding_surface_,
@@ -818,6 +833,9 @@
                              MFVideoInterlace_MixedInterlaceOrProgressive);
   RETURN_ON_HR_FAILURE(hr, "Failed to set interlace mode", false);
 
+  hr = MFSetAttributeSize(media_type, MF_MT_PIXEL_ASPECT_RATIO, 2, 1);

[Ami GONE FROM CHROMIUM, 2012/11/13 23:07:13]

wat?

[ananta, 2012/11/13 23:17:22]

Unrelated change. Was debugging the small video painting bug. removed.

+  RETURN_ON_HR_FAILURE(hr, "Failed to set aspect ratio", false);
+
   hr = decoder_->SetInputType(0, media_type, 0);  // No flags
   RETURN_ON_HR_FAILURE(hr, "Failed to set decoder input type", false);
   return true;