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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/gpu/media/dxva_video_decode_accelerator.h"
 
 #if !defined(OS_WIN)
 #error This file should only be built on Windows.
 #endif   // !defined(OS_WIN)
 
@@ skipping 55 matching lines @@
       StopOnError(error_code);                                      \
       return ret;                                                   \
     }                                                               \
   } while (0)
 
 #define RETURN_AND_NOTIFY_ON_HR_FAILURE(result, log, error_code, ret)  \
   RETURN_AND_NOTIFY_ON_FAILURE(SUCCEEDED(result),                      \
                                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());
   RETURN_ON_HR_FAILURE(hr, "MFCreateSample failed", NULL);
   return sample.Detach();
 }
 
 // Creates a Media Foundation sample with one buffer of length |buffer_length|
 // on a |align|-byte boundary. Alignment must be a perfect power of 2 or 0.
 static IMFSample* CreateEmptySampleWithBuffer(int buffer_length, int align) {
@@ skipping 42 matching lines @@
   DWORD max_length = 0;
   DWORD current_length = 0;
   uint8* destination = NULL;
   hr = buffer->Lock(&destination, &max_length, &current_length);
   RETURN_ON_HR_FAILURE(hr, "Failed to lock buffer", NULL);
 
   CHECK_EQ(current_length, 0u);
   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();
 }
 
 static IMFSample* CreateSampleFromInputBuffer(
     const media::BitstreamBuffer& bitstream_buffer,
     DWORD stream_size,
     DWORD alignment) {
   base::SharedMemory shm(bitstream_buffer.handle(), true);
   RETURN_ON_FAILURE(shm.Map(bitstream_buffer.size()),
                     "Failed in base::SharedMemory::Map", NULL);
@@ skipping 235 matching lines @@
                               NULL,
                               d3d_surface,
                               NULL,
                               D3DTEXF_NONE);
     RETURN_ON_HR_FAILURE(hr, "Colorspace conversion via StretchRect failed",
                          false);
     // Ideally, this should be done immediately before the draw call that uses
     // 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_,
         EGL_BACK_BUFFER);
   } else {
     scoped_array<char> bits;
     RETURN_ON_FAILURE(GetBitmapFromSurface(DXVAVideoDecodeAccelerator::device_,
                                            dest_surface, &bits),
                       "Failed to get bitmap from surface for rendering",
                       false);
@@ skipping 397 matching lines @@
 
   hr = media_type->SetGUID(MF_MT_SUBTYPE, MFVideoFormat_H264);
   RETURN_ON_HR_FAILURE(hr, "Failed to set subtype", false);
 
   // Not sure about this. msdn recommends setting this value on the input
   // media type.
   hr = media_type->SetUINT32(MF_MT_INTERLACE_MODE,
                              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;
 }
 
 bool DXVAVideoDecodeAccelerator::SetDecoderOutputMediaType(
     const GUID& subtype) {
   base::win::ScopedComPtr<IMFMediaType> out_media_type;
 
   for (uint32 i = 0;
@@ skipping 307 matching lines @@
       return;
   }
 
   MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
       &DXVAVideoDecodeAccelerator::NotifyFlushDone, base::AsWeakPtr(this)));
 
   state_ = kNormal;
 }
 
 }  // namespace content