Make sure MediaStreamVideoSource support cropping.

content/renderer/media/media_stream_video_source.cc

 // Copyright 2014 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/renderer/media/media_stream_video_source.h"
 
+#include <algorithm>
 #include <limits>
 #include <string>
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "content/renderer/media/media_stream_dependency_factory.h"
 #include "content/renderer/media/webrtc/webrtc_video_capturer_adapter.h"
 
 namespace content {
 
@@ skipping 12 matching lines @@
 const int MediaStreamVideoSource::kDefaultFrameRate = 30;
 
 namespace {
 // Constraints keys for http://dev.w3.org/2011/webrtc/editor/getusermedia.html
 const char kSourceId[] = "sourceId";
 
 // Google-specific key prefix. Constraints with this prefix are ignored if they
 // 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.
 bool UpdateFormatForConstraint(
     const blink::WebMediaConstraint& constraint,
     bool mandatory,
     media::VideoCaptureFormat* format) {
   DCHECK(format != NULL);
 
   if (!format->IsValid())
@@ skipping 40 matching lines @@
 
   int value;
   if (!base::StringToInt(constraint_value, &value)) {
     DLOG(WARNING) << "Can't parse MediaStream constraint. Name:"
                   <<  constraint_name << " Value:" << constraint_value;
     return false;
   }
   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) {
     if (value == 0) {
       // The frame rate is set by constraint.
       // Don't allow 0 as frame rate if it is a mandatory constraint.
       // Set the frame rate to 1 if it is not mandatory.
       if (mandatory) {
         return false;
       } else {
@@ skipping 64 matching lines @@
     FilterFormatsByConstraint(optional[i], false, &current_candidates);
     if (!current_candidates.empty()) {
       candidates = current_candidates;
     }
   }
 
   // We have done as good as we can to filter the supported resolutions.
   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;
     }
   }
   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(
     MediaStreamDependencyFactory* factory)
     : state_(NEW),
       factory_(factory),
       capture_adapter_(NULL) {
   DCHECK(factory_);
 }
 
@@ skipping 23 matching lines @@
 
       state_ = RETRIEVING_CAPABILITIES;
       GetCurrentSupportedFormats(max_requested_width,
                                  max_requested_height);
 
       break;
     }
     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:
     case STARTED: {
       // Currently, reconfiguring the source is not supported.
       FinalizeAddTrack();
     }
   }
 }
 
@@ skipping 26 matching lines @@
 }
 
 void MediaStreamVideoSource::DoStopSource() {
   DVLOG(3) << "DoStopSource()";
   StopSourceImpl();
   state_ = ENDED;
 }
 
 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(
     const media::VideoCaptureFormats& formats) {
   DCHECK(CalledOnValidThread());
   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;
   }
 
   state_ = STARTING;
   DVLOG(3) << "Starting the capturer with"
            << " width = " << current_format_.frame_size.width()
            << " height = " << current_format_.frame_size.height()
            << " frame rate = " << current_format_.frame_rate;
 
   media::VideoCaptureParams params;
   params.requested_format = current_format_;
   StartSourceImpl(params);
 }
 
 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) {
     const blink::WebMediaConstraints& requested_constraints =
         request_it->constraints;
 
     media::VideoCaptureFormats filtered_formats =
         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;
     }
   }
   return false;
 }
 
 void MediaStreamVideoSource::OnStartDone(bool success) {
   DCHECK(CalledOnValidThread());
   DVLOG(3) << "OnStartDone({success =" << success << "})";
@@ skipping 37 matching lines @@
 MediaStreamVideoSource::RequestedConstraints::RequestedConstraints(
     const blink::WebMediaConstraints& constraints,
     const ConstraintsCallback& callback)
     : constraints(constraints), callback(callback) {
 }
 
 MediaStreamVideoSource::RequestedConstraints::~RequestedConstraints() {
 }
 
 }  // namespace content