Win video capture: Fix capture format and frame buffer mismatch

content/browser/renderer_host/media/video_capture_device_client.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.

[mcasas, 2016/09/27 14:36:02]

There's no need to specify the project
in the bug line: BUG=650382 is enough.

[magjed_chromium, 2016/09/27 15:19:56]

Done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/media/video_capture_device_client.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 198 matching lines @@
       break;
     case media::PIXEL_FORMAT_MJPEG:
       origin_colorspace = libyuv::FOURCC_MJPG;
       break;
     default:
       NOTREACHED();
   }
 
   // The input |length| can be greater than the required buffer size because of
   // paddings and/or alignments, but it cannot be smaller.
-  DCHECK_GE(static_cast<size_t>(length), frame_format.ImageAllocationSize());
+  if (static_cast<size_t>(length) < frame_format.ImageAllocationSize()) {

[mcasas, 2016/09/27 14:36:01]

IIUC, the DCHECK has never been hit before 
except for this particular devices, but the 
bug says that ManyCam doesn't work at all 
anyway. If so, the correct handling is to 
blacklist it as we do in [1] -- which I 
totally thought we already did for ManyCam.


[1]
https://cs.chromium.org/chromium/src/media/capture/video/win/video_capture_de...

[magjed_chromium, 2016/09/27 15:19:56]

ManyCam doesn't work for the bug reporter in any browser. ManyCam typically
works though, I have tried it myself on Windows 8 and on my MacBook. I think
it's better to drop frames in these cases than to crash the browser.

[mcasas, 2016/09/28 16:39:22]

I'm not sure this is the right thing to do. 
1- since both |length| and |frame_format| are input 
parameters, this DCHECK should happen at the start of 
the method. 

2- the bug description essentially points to some 
corrupted driver's memory (?) being funky when 
accessed, so how's this change going to help?

3- this problem affects Windows only Manycams,
so I'd strongly encourage addressing this issue, if
anywhere,  in SinkInputPin::Receive(...) or 
VideoCaptureDeviceWin::FrameReceived(...). In
those locations we receive data but don't validate
its length against the expected |capture_format_|
length, which is configured previously in time.

Finally, a bit of accountancy: this CL's bug
https://crbug.com/650382 is marked duped of
https://crbugcom/613556 but the latter was closed 
as WontFix, I guess one of them is wrongly labelled.

[magjed_chromium, 2016/09/29 11:00:35]

1. That's an unrelated change, but sure, I moved the DCHECK to the top of the
method.

2. This change is going to help by not crashing the browser. Video will still
not work of course.

3. Ok, I moved it into Win specific code. Let's hope it doesn't happen on other
platforms.

Regarding the bugs: 650382 has not been marked a duplicate of 613556 yet, it's
still an open bug. I will group them together now.

[mcasas, 2016/09/29 20:43:31]

The crash is only present in Win, and Manycam is available
in Win and Mac only. 

+    DLOG(WARNING) << "Frame format: "
+                  << media::VideoCaptureFormat::ToString(frame_format)
+                  << " requires at least " << frame_format.ImageAllocationSize()
+                  << " bytes, but only " << length << " bytes was given.";

[mcasas, 2016/09/27 14:36:01]

Who is going to read this DLOG anyway? Code
in media/ seems to be verbose for no reason
already.

[magjed_chromium, 2016/09/27 15:19:56]

I don't know if anyone will use it. We can remove the log and just return if you
prefer that.

[mcasas, 2016/09/28 16:39:22]

Yes, by all means, but probably you would revert 
this file's changes in light of my comments before.

+    return;
+  }
 
   if (external_jpeg_decoder_) {
     const VideoCaptureGpuJpegDecoder::STATUS status =
         external_jpeg_decoder_->GetStatus();
     if (status == VideoCaptureGpuJpegDecoder::FAILED) {
       external_jpeg_decoder_.reset();
     } else if (status == VideoCaptureGpuJpegDecoder::INIT_PASSED &&
         frame_format.pixel_format == media::PIXEL_FORMAT_MJPEG &&
         rotation == 0 && !flip) {
       external_jpeg_decoder_->DecodeCapturedData(data, length, frame_format,
@@ skipping 175 matching lines @@
           reinterpret_cast<uint8_t*>(buffer->data(VideoFrame::kUPlane));
       *v_plane_data =
           reinterpret_cast<uint8_t*>(buffer->data(VideoFrame::kVPlane));
       return buffer;
   }
   NOTREACHED();
   return std::unique_ptr<Buffer>();
 }
 
 }  // namespace content