Make sure MediaStreamVideoSource support cropping.

content/renderer/media/media_stream_video_source.cc

Index: content/renderer/media/media_stream_video_source.cc
diff --git a/content/renderer/media/media_stream_video_source.cc b/content/renderer/media/media_stream_video_source.cc
index 118d66e3cddc59faa72ed1fd4c7448c0bc0c377e..ef740a98daa224223ff8a58eae3efc4c0d66799d 100644
--- a/content/renderer/media/media_stream_video_source.cc
+++ b/content/renderer/media/media_stream_video_source.cc
@@ -4,6 +4,7 @@
 
 #include "content/renderer/media/media_stream_video_source.h"
 
+#include <algorithm>
 #include <limits>
 #include <string>
 
@@ -36,6 +37,10 @@ const char kSourceId[] = "sourceId";
 // are unknown.
 const char kGooglePrefix[] = "goog";
 
+// MediaStreamVideoSource supports cropping of video frames but only up to
+// kMaxCropFactor.

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

I'm not understanding this.  Does this mean that (cx*cy)/2>=
cropped_cx*cropped_cy?
Does it account for aspect ratio?

[perkj_chrome, 2014/03/06 12:45:42]

Please see new comment.

+const int kMaxCropFactor = 2;
+
 // Returns true if |constraint| is fulfilled. |format| can be changed
 // changed by a constraint. Ie - the frame rate can be changed by setting
 // maxFrameRate.
@@ -96,11 +101,11 @@ bool UpdateFormatForConstraint(
   if (constraint_name == MediaStreamVideoSource::kMinWidth) {
     return (value <= format->frame_size.width());
   } else if (constraint_name == MediaStreamVideoSource::kMaxWidth) {
-    return (value >= format->frame_size.width());
+    return (value * kMaxCropFactor >= format->frame_size.width());
   } else if (constraint_name == MediaStreamVideoSource::kMinHeight) {
     return (value <= format->frame_size.height());
   } else if (constraint_name == MediaStreamVideoSource::kMaxHeight) {
-    return (value >= format->frame_size.height());
+     return (value * kMaxCropFactor >= format->frame_size.height());
   } else if (constraint_name == MediaStreamVideoSource::kMinFrameRate) {
     return (value <= format->frame_rate);
   } else if (constraint_name == MediaStreamVideoSource::kMaxFrameRate) {
@@ -185,25 +190,43 @@ media::VideoCaptureFormats FilterFormats(
   return candidates;
 }
 
-// Find the format that best matches the default video size.
-// This algorithm is chosen since a resolution must be picked even if no
-// constraints are provided. We don't just select the maximum supported
-// resolution since higher resolution cost more in terms of complexity and
-// many cameras perform worse at its maximum supported resolution.
-const media::VideoCaptureFormat& GetBestCaptureFormat(
-    const media::VideoCaptureFormats& formats) {
-  DCHECK(!formats.empty());
+// Retrieve the desired max width and height from |constraints|.
+void GetDesiredMaxWidthAndHeight(const blink::WebMediaConstraints& constraints,
+                                 int* dw, int* dh) {

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

full variable names

[perkj_chrome, 2014/03/06 12:45:42]

Done.

[tommi (sloooow) - chröme, 2014/03/06 13:28:50]

Thanks... at first I thought the 'd' stood for delta.  So this a good change.

[perkj_chrome, 2014/03/06 14:22:53]

Done.

+  bool mandatory_found = false;
+  blink::WebString width;
+  if (constraints.getMandatoryConstraintValue(
+          MediaStreamVideoSource::kMaxWidth, width)) {
+    base::StringToInt(width.utf8(), dw);
+    mandatory_found = true;
+  }
+  blink::WebString height;
+  if (constraints.getMandatoryConstraintValue(
+          MediaStreamVideoSource::kMaxHeight, height)) {
+    base::StringToInt(height.utf8(), dh);
+    mandatory_found = true;
+  }
+  if (mandatory_found)

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

What if height is specified as mandatory and width as optional - is there any
reason we would not want to pick up the width?

I actually don't see much value in the 'mandatory_found' variable in this
function or separate height and width strings.
Why wouldn't we simply do this:

blink::WebString web_string;
if (constraints.getMandatoryConstraintValue(
        MediaStreamVideoSource::kMaxWidth, web_string) ||
    constraints.getOptionalConstraintValue(
        MediaStreamVideoSource::kMaxWidth, web_string)) {
  base::StringToInt(web_string.utf8(), dw);
}

if (constraints.getMandatoryConstraintValue(
        MediaStreamVideoSource::kMaxHeight, web_string) ||
    constraints.getOptionalConstraintValue(
        MediaStreamVideoSource::kMaxHeight, web_string)) {
  base::StringToInt(web_string.utf8(), dh);
}

[perkj_chrome, 2014/03/06 12:45:42]

I did that first.
The whole think gets kind of complicated fast. The idea is to use the filtering
mechanism of mandatory constraints and optional constraints against the "native
resolution" first and then crop if requested. It feels a bit scary to treat
maxWidht from the mandatory set and maxHeight from the optional set. I feels
like a higher risk of breaking the expected aspect ratio. Therefore I decided to
simply ignore the optional if a mandatory constraint has been given.  That is at
least not wrong since optional is optional.

[tommi (sloooow) - chröme, 2014/03/06 13:28:50]

OK that's probably the safe thing to do.

Btw, you could reduce code duplication in this function by having a utility
function at the top of this file:

bool GetConstraintValue(const blink::WebMediaConstraints& constraints,
      bool mandatory, const char* name, int* value) {
  blink::WebString value_str;
  bool ret = mandatory ?
      constraints.getMandatoryConstraintValue(name, value_str) :
      constraints.getOptionalConstraintValue(name, value_str);
  if (ret)
    *value = base::StringToInt(value.utf8(), value);
  return ret;
}


and then here the code will be a little easier to follow:

bool mandatory = GetConstraintValue(constraints, true,
MediaStreamVideoSource::kMaxWidth, desired_width);
mandatory |= GetConstraintValue(constraints, true,
MediaStreamVideoSource::kMaxHeight, desired_height);
if (mandatory)
  return;

GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxWidth,
desired_width);
GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxHeight,
desired_height);

[perkj_chrome, 2014/03/06 14:22:53]

For some reason I get a compile error with  const char* name.
but not if I do const blink::WebString& name. I don't understand why it matters
where the conversion between  WebString and const char * is done but.... 

On 2014/03/06 13:28:50, tommi wrote:

+    return;
 
-  int default_area =
-      MediaStreamVideoSource::kDefaultWidth *
-      MediaStreamVideoSource::kDefaultHeight;
+  if (constraints.getOptionalConstraintValue(
+          MediaStreamVideoSource::kMaxWidth, width)) {
+      base::StringToInt(width.utf8(), dw);
+  }
+  if (constraints.getOptionalConstraintValue(
+          MediaStreamVideoSource::kMaxHeight, height)) {
+    base::StringToInt(height.utf8(), dh);
+  }
+}
 
+const media::VideoCaptureFormat& GetBestFormatBasedOnArea(
+    const media::VideoCaptureFormats& formats,
+    int area) {
   media::VideoCaptureFormats::const_iterator it = formats.begin();
   media::VideoCaptureFormats::const_iterator best_it = formats.begin();
   int best_diff = std::numeric_limits<int>::max();
   for (; it != formats.end(); ++it) {
-    int diff = abs(default_area -
-                   it->frame_size.width() * it->frame_size.height());
+    int diff = abs(area - it->frame_size.width() * it->frame_size.height());
     if (diff < best_diff) {
       best_diff = diff;
       best_it = it;
@@ -212,6 +235,41 @@ const media::VideoCaptureFormat& GetBestCaptureFormat(
   return *best_it;
 }
 
+// Find the format that best matches the default video size.
+// This algorithm is chosen since a resolution must be picked even if no
+// constraints are provided. We don't just select the maximum supported
+// resolution since higher resolution cost more in terms of complexity and

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

nit: resolutions

[perkj_chrome, 2014/03/06 12:45:42]

Done.

+// many cameras perform worse at its maximum supported resolution.

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

its -> their

Regarding the performance statement itself, how do we measure camera performance
in this context?
I know that the frame sizes will of course be bigger but how does the camera
actually perform worse?

[perkj_chrome, 2014/03/06 12:45:42]

 frame rate is lower etc. Updated comment.

+void GetBestCaptureFormat(
+    const media::VideoCaptureFormats& formats,
+    const blink::WebMediaConstraints& constraints,
+    media::VideoCaptureFormat* capture_format,
+    gfx::Size* frame_output_size) {
+  DCHECK(!formats.empty());
+  DCHECK(frame_output_size);
+
+  int max_width = std::numeric_limits<int>::max();
+  int max_height = std::numeric_limits<int>::max();;
+  GetDesiredMaxWidthAndHeight(constraints, &max_width, &max_height);
+
+  *capture_format = GetBestFormatBasedOnArea(
+      formats,
+      std::min(max_width, MediaStreamVideoSource::kDefaultWidth) *
+      std::min(max_height, MediaStreamVideoSource::kDefaultHeight));
+
+  *frame_output_size = capture_format->frame_size;
+  if (max_width < frame_output_size->width())
+    frame_output_size->set_width(max_width);
+  if (max_height < frame_output_size->height())
+    frame_output_size->set_height(max_height);
+}
+
+// Empty method used for keeping a reference to the original media::VideoFrame
+// in MediaStreamVideoSource::DeliverVideoFrame if cropping is needed.

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

I don't understand this actually :)
The method doesn't do anything, so how can it be used for keeping a reference?

[perkj_chrome, 2014/03/06 12:45:42]

See base::Bind(&ReleaseOriginalFrame, frame)) further down. Do you have a better
idea how do do frame->AddRef and frame->Release() bounded to callback? I asked
Joi and Alpha.

[tommi (sloooow) - chröme, 2014/03/06 13:28:50]

Ah, I missed that.  Can you add something to the comments that explain that we
keep the reference in the closure that eventually calls this method?

[perkj_chrome, 2014/03/06 14:22:53]

Done.

+void ReleaseOriginalFrame(
+    const scoped_refptr<media::VideoFrame>& frame) {
+}
+
 }  // anonymous namespace
 
 MediaStreamVideoSource::MediaStreamVideoSource(
@@ -255,7 +313,7 @@ void MediaStreamVideoSource::AddTrack(
     case STARTING:
     case RETRIEVING_CAPABILITIES: {
       // The |callback| will be triggered once the delegate has started or
-      // the capabilitites has been retrieved.
+      // the capabilities has been retrieved.

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

since you're fixing this you might as well fix has->have

[perkj_chrome, 2014/03/06 12:45:42]

Done.

       break;
     }
     case ENDED:
@@ -302,8 +360,42 @@ void MediaStreamVideoSource::DoStopSource() {
 
 void MediaStreamVideoSource::DeliverVideoFrame(
     const scoped_refptr<media::VideoFrame>& frame) {
-  if (capture_adapter_)
-    capture_adapter_->OnFrameCaptured(frame);
+  scoped_refptr<media::VideoFrame> video_frame(frame);
+
+  if (frame->visible_rect().size() != frame_output_size_) {
+    // If |frame| is not the size that is expected, we need to crop it by
+    // providing a new |visible_rect|. The new visible rect must be within the
+    // original |visible_rect|.
+    const int visible_width = std::min(frame_output_size_.width(),
+                                       frame->visible_rect().width());
+
+    // TODO(perkj): horiz_crop and vert_crop must be 0 until local
+    // rendering can support offsets on media::VideoFrame::visible_rect().
+    // crbug/349450. The effect of this is that the cropped frame originate

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

nit: originates

[perkj_chrome, 2014/03/06 12:45:42]

Done.

+    // from the top left of the original frame instead of being centered.
+    // Find a new horizontal offset within |frame|.
+    //const int horiz_crop = frame->visible_rect().x() +
+        // ((frame->visible_rect().width() - visible_width) / 2);

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

align //

[perkj_chrome, 2014/03/06 12:45:42]

Done.

+    // Find a new vertical offset within |frame|.
+    // const int vert_crop = frame->visible_rect().y() +
+    //       ((frame->visible_rect().height() - visible_height) / 2);
+    const int horiz_crop = 0;
+    const int vert_crop = 0;
+
+    const int visible_height = std::min(frame_output_size_.height(),
+                                        frame->visible_rect().height());
+
+    gfx::Rect rect(horiz_crop, vert_crop, visible_width,
+                   visible_height);

[tommi (sloooow) - chröme, 2014/03/06 10:14:09]

nit: this should be indented by 4 spaces

[perkj_chrome, 2014/03/06 12:45:42]

Done.

+    video_frame = media::VideoFrame::WrapVideoFrame(
+        frame, rect, base::Bind(&ReleaseOriginalFrame, frame));
+  }
+
+  if ((frame->format() == media::VideoFrame::I420 ||
+       frame->format() == media::VideoFrame::YV12) &&
+      capture_adapter_) {
+    capture_adapter_->OnFrameCaptured(video_frame);
+  }
 }
 
 void MediaStreamVideoSource::OnSupportedFormats(
@@ -312,8 +404,10 @@ void MediaStreamVideoSource::OnSupportedFormats(
   DCHECK_EQ(RETRIEVING_CAPABILITIES, state_);
 
   supported_formats_ = formats;
-  if (!FindBestFormatWithConstraints(supported_formats_, &current_format_,
-                      &current_constraints_)) {
+  if (!FindBestFormatWithConstraints(supported_formats_,
+                                     &current_format_,
+                                     &frame_output_size_,
+                                     &current_constraints_)) {
     FinalizeAddTrack();
     SetReadyState(blink::WebMediaStreamSource::ReadyStateEnded);
     return;
@@ -333,8 +427,9 @@ void MediaStreamVideoSource::OnSupportedFormats(
 bool MediaStreamVideoSource::FindBestFormatWithConstraints(
     const media::VideoCaptureFormats& formats,
     media::VideoCaptureFormat* best_format,
+    gfx::Size* frame_output_size,
     blink::WebMediaConstraints* resulting_constraints) {
-  // Find the first constraints that we can fulfilled.
+  // Find the first constraints that we can fulfill.
   for (std::vector<RequestedConstraints>::iterator request_it =
            requested_constraints_.begin();
        request_it != requested_constraints_.end(); ++request_it) {
@@ -345,7 +440,10 @@ bool MediaStreamVideoSource::FindBestFormatWithConstraints(
         FilterFormats(requested_constraints, formats);
     if (filtered_formats.size() > 0) {
       // A request with constraints that can be fulfilled.
-      *best_format = GetBestCaptureFormat(filtered_formats);
+      GetBestCaptureFormat(filtered_formats,
+                           requested_constraints,
+                           best_format,
+                           frame_output_size);
       *resulting_constraints= requested_constraints;
       return true;
     }