Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
558623002:
Video capture: Refactor GetBestMatchedFormat from Win to OS independent (Closed)
|
Created:
6 years, 3 months ago by magjed_chromium Modified:
6 years, 2 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionVideo capture: Refactor GetBestMatchedFormat
Committed: https://crrev.com/04320485ac67d967ead8f4fc267c254293f33ba7
Cr-Commit-Position: refs/heads/master@{#299499}
Patch Set 1 #Patch Set 2 : fix mac syntax error #
Total comments: 11
Patch Set 3 : tommi@s and mcasas@ comments #Patch Set 4 : Change diff function to handle rational frame rates #Patch Set 5 : rebase #
Total comments: 7
Patch Set 6 : mcasas@ comments #
Total comments: 2
Patch Set 7 : change diff type from int to float #
Total comments: 4
Patch Set 8 : preserve original behaviour #
Total comments: 6
Patch Set 9 : sort pixel formats in order of preference #
Total comments: 10
Patch Set 10 : offline discussion and revert pixel format order #Patch Set 11 : replace std::min_element and std::bind with loop #
Total comments: 6
Patch Set 12 : remove braces in if statement #
Messages
Total messages: 43 (14 generated)
magjed@chromium.org changed reviewers: + mcasas@chromium.org, tommi@chromium.org
Please take a look.
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/558623002/diff/40001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_mac.mm:54: static const gfx::Size kQVGA(320, 240), Do these require static initialization? (i.e. code that executes as part of global initialization) If so, the cl is going to get reverted after landing (we have perf bots that catch that and turn red) https://codereview.chromium.org/558623002/diff/40001/media/video/capture/vide... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/558623002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_types.cc:84: int diff_width; make all of these const? https://codereview.chromium.org/558623002/diff/40001/media/video/capture/vide... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/558623002/diff/40001/media/video/capture/vide... media/video/capture/video_capture_types.h:82: class CompareVideoFormat { Is it possible to keep this in the cc file only? https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/... media/video/capture/win/capability_list_win.cc:21: return compare_video_format_(lhs.second, rhs.second); is there a way to avoid using operator overloading? https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_mf_win.cc (right): https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_mf_win.cc:73: stream_index++; ++stream_index;
I might not see clearly the reason for all these new classes...
What about adding a static method to video_capture_types.cc like
index/iterator FindClosestFormat(
const VideoCaptureFormats& haystack,
const VideoCaptureFormat& needle);
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_types.h:84: CompareVideoFormat(const
VideoCaptureFormat& requested);
explicit
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.cc (right):
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:37: static bool
GetFrameRate(IMFMediaType* type, float* frame_rate) {
const IMFMediaType* type ?
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:48: static bool
FillCapabilitiesFromType(IMFMediaType* type,
s/FillCapabilities/FillFormat/
const IMFMediaType* type
just adding info on COM specifics. The cleanup in this CL generally looks good to me fwiw. https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/... File media/video/capture/win/video_capture_device_mf_win.cc (right): https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/... media/video/capture/win/video_capture_device_mf_win.cc:37: static bool GetFrameRate(IMFMediaType* type, float* frame_rate) { On 2014/09/10 16:52:28, mcasas wrote: > const IMFMediaType* type ? COM doesn't have a concept of 'const' for objects and methods. Hence if this was made const, the implementation would not be able to hold on to a reference to it (since it can't call AddRef/Release). So, when passing around naked pointers to COM objects, don't use const. What you could do instead would be to pass |const ScopedComPtr<IMFMediaType>& type|. This would be in line with how we typically pass reference counted objects.
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
I reverted most changes in video_capture_device_mac.mm; trying to reuse code
there generates more code than it removes.
Regarding:
index/iterator FindClosestFormat(
const VideoCaptureFormats& haystack,
const VideoCaptureFormat& needle);
That would indeed be a simpler interface. But sometimes each VideoCaptureFormat
is associated with an OS specific identifier (stream_index in windows and
IDeckLinkDisplayMode in mac) and then you want to find not just the
VideoCaptureFormat but also the identifier. In that case you need to generate a
temporary list of VideoCaptureFormats, find the index with FindClosestFormat,
and then try to find the identifier again using the index. It would be better to
compare formats on the fly while you iterate.
I changed the implementation so that DiffVideoCaptureFormat is just an int
instead. Then we can compare with '<' without overloading operators.
Also, should I ignore files in third_party/webrtc/modules/video_capture? It is
possible to remove > 100 lines of code from GetBestMatchedCapability in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/mac/...
File media/video/capture/mac/video_capture_device_mac.mm (right):
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/mac/...
media/video/capture/mac/video_capture_device_mac.mm:54: static const gfx::Size
kQVGA(320, 240),
On 2014/09/09 20:11:10, tommi wrote:
> Do these require static initialization? (i.e. code that executes as part of
> global initialization)
> If so, the cl is going to get reverted after landing (we have perf bots that
> catch that and turn red)
Ok, cool. I don't think it's possible to remove the constructors from global
initialization without using C++11 features (list initialization or constexpr),
so I reverted this.
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/...
File media/video/capture/win/video_capture_device_mf_win.cc (right):
https://codereview.chromium.org/558623002/diff/40001/media/video/capture/win/...
media/video/capture/win/video_capture_device_mf_win.cc:73: stream_index++;
On 2014/09/09 20:11:11, tommi wrote:
> ++stream_index;
Done.
Ping. Can you please take another look?
Couple of comments. Could you add perkj@? https://codereview.chromium.org/558623002/diff/180001/media/video/capture/vid... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/558623002/diff/180001/media/video/capture/vid... media/video/capture/video_capture_types.cc:79: // precedence over frame rate. This comment doesn't read clearly to me, can you please rewrite? (specially if you take the next comment in) https://codereview.chromium.org/558623002/diff/180001/media/video/capture/vid... media/video/capture/video_capture_types.cc:84: std::fabs(lhs.frame_rate - rhs.frame_rate)); what about: return kFrameRatePrecision * (std::abs(lhs.frame_size.width() - rhs.frame_size.width()) + std::abs(lhs.frame_size.height() - rhs.frame_size.height()) + static_cast<int>(std::fabs(lhs.frame_rate - rhs.frame_rate))); https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:9: CapabilityList::const_iterator GetBestMatchedFormat( Rename to GetBestMatchedCapability() ? https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:536: // a util method written. Can't assume the first value will return Please rewrite doco. ("So a util method written", "eg" instead of "e.g." etc)
took a brief look and this lgtm but I think Miguel will have better insights.
magjed@chromium.org changed reviewers: + perkj@chromium.org
+perkj. Per, can you please take a look as well? https://codereview.chromium.org/558623002/diff/180001/media/video/capture/vid... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/558623002/diff/180001/media/video/capture/vid... media/video/capture/video_capture_types.cc:84: std::fabs(lhs.frame_rate - rhs.frame_rate)); On 2014/09/23 12:34:57, mcasas wrote: > what about: > > return kFrameRatePrecision * > (std::abs(lhs.frame_size.width() - rhs.frame_size.width()) + > std::abs(lhs.frame_size.height() - rhs.frame_size.height()) + > static_cast<int>(std::fabs(lhs.frame_rate - rhs.frame_rate))); > The reason for not factoring kFrameRatePrecision is that we have both a float multiplication and an int multiplication. Your suggestion does not work because you have already lost the precision in the frame rate and are just blowing up an integer. The alternative is to convert everything to float, then multiply, then convert to int. I implemented that change. https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:9: CapabilityList::const_iterator GetBestMatchedFormat( On 2014/09/23 12:34:57, mcasas wrote: > Rename to GetBestMatchedCapability() ? Done. https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/558623002/diff/180001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:536: // a util method written. Can't assume the first value will return On 2014/09/23 12:34:57, mcasas wrote: > Please rewrite doco. ("So a util method written", "eg" instead of "e.g." etc) Done. I removed that section of comments. I think it is more clearly phrased in the next section of comments already.
LGTM modulo my comment below. Hate all those float to int casts :) https://codereview.chromium.org/558623002/diff/200001/media/video/capture/vid... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/558623002/diff/200001/media/video/capture/vid... media/video/capture/video_capture_types.cc:79: return static_cast<int>( Gotcha. However, I'm going to say, why not working in float? This is not going to be a per-frame operation.
https://codereview.chromium.org/558623002/diff/200001/media/video/capture/vid... File media/video/capture/video_capture_types.cc (right): https://codereview.chromium.org/558623002/diff/200001/media/video/capture/vid... media/video/capture/video_capture_types.cc:79: return static_cast<int>( On 2014/09/23 14:45:32, mcasas wrote: > Gotcha. However, I'm going to say, why not working in float? > This is not going to be a per-frame operation. Done.
nice cleanup but I don't like the change in behaviour. https://codereview.chromium.org/558623002/diff/220001/media/video/capture/vid... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/558623002/diff/220001/media/video/capture/vid... media/video/capture/video_capture_types.h:101: float DiffVideoCaptureFormat(const VideoCaptureFormat& lhs, Is it possible to place this method somewhere else? I suggest video_capture_device.h This header file is included a lot of different files in content/renderer as well. https://codereview.chromium.org/558623002/diff/220001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (left): https://codereview.chromium.org/558623002/diff/220001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:117: return *diff_list.front().capability; This also sort the color format. We prefer planar yuv formats since the color conversions are then much cheaper. https://codereview.chromium.org/558623002/diff/220001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/220001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:15: if (DiffVideoCaptureFormat(requested, it->supported_format) < So this would be a behavior change. Currently we sort: 1. Find best matching height 2. Find best matching width 3. Find best matching frame rate. 4. Find best color format. This new algorithm finds the best matching pixels/time unit. I think that we should treat frame rate separate from resolution. With higher resolution, the frame rate is often lower. So if a higher resolution is requested, I think that is what we should do even if the frame rate is lower. Also, we should pick the color format that is cheapest. But it must be done after matching resolution and frame rate since for example mjpg give us a higher frame rate but is much more expensive to convert to I420.
Regarding preserving behaviour: I started off doing that, but after looking at 8
different implementations in the codebase, all different, I figured I could
choose whichever. To be most consistent with Linux and Android, we should store
stream_index in the requested format for Windows and just try to open it,
otherwise fail. If nothing unexpected happens, we expect to find a perfect match
anyway, right? That is how it is done in Linux and Android. In Linux, we also
sort on color format, but it is not the same sort as in Windows. In Linux, for
large resolutions, we favour mjpeg over raw formats. Why? Should I implement
that for Windows as well?
These are the 8 different implementations I found in the codebase:
* Windows:
const VideoCaptureCapabilityWin& GetBestMatchedFormat(
int requested_width,
int requested_height,
float requested_frame_rate) const;
Prioritize closest height, width, and frame rate in that order. If we still have
a tie, prioritize color format according to the list of precedence: enum
media::VideoPixelFormat.
* Android: Just try to open the requested format.
* Linux: Just try to open the requested resolution and frame rate. Choose color
format according to the list of precedence:
VideoCaptureDeviceLinux::GetListOfUsableFourCCs.
* Mac, media/video/capture/mac/video_capture_device_mac.mm:
GetBestMatchSupportedResolution
Prioritize closest resolution area. Clamp frame rate. Set color format to UYVY.
* Mac DeckLink, media/video/capture/mac/video_capture_device_decklink_mac.mm:
Prioritize according to L1 distance for resolution and frame rate.
* third_party/libjingle/source/talk/media/base/videocapturer.cc:
int64 cricket::VideoCapturer::GetFormatDistance(const VideoFormat& desired,
const VideoFormat& supported)
Very complex with penalties and stuff.
* third_party/libjingle/source/talk/app/webrtc/videosource.cc:
const cricket::VideoFormat& GetBestCaptureFormat(
const std::vector<cricket::VideoFormat>& formats)
Sort on closest resolution area, then frame rate.
* third_party/webrtc/modules/video_capture/device_info_impl.cc:
int32_t webrtc::videocapturemodule::DeviceInfoImpl::GetBestMatchedCapability(
const char*deviceUniqueIdUTF8,
const VideoCaptureCapability& requested,
VideoCaptureCapability& resulting)
Prioritization is done according to this algorithm:
1) Height: As big as possible if < requested, otherwise as close to requested as
possible
2) Width: As big as possible if < requested, otherwise as close to requested as
possible
3) Framerate: As high as possible if < requested, otherwise as close to
requested as possible
https://codereview.chromium.org/558623002/diff/220001/media/video/capture/vid...
File media/video/capture/video_capture_types.h (right):
https://codereview.chromium.org/558623002/diff/220001/media/video/capture/vid...
media/video/capture/video_capture_types.h:101: float
DiffVideoCaptureFormat(const VideoCaptureFormat& lhs,
On 2014/09/24 07:56:29, perkj wrote:
> Is it possible to place this method somewhere else? I suggest
> video_capture_device.h
> This header file is included a lot of different files in content/renderer as
> well.
Will fix in next upload.
On 2014/09/24 10:33:25, magjed wrote: > Regarding preserving behaviour: I started off doing that, but after looking at 8 > different implementations in the codebase, all different, I figured I could > choose whichever. To be most consistent with Linux and Android, we should store > stream_index in the requested format for Windows and just try to open it, > otherwise fail. If nothing unexpected happens, we expect to find a perfect match > anyway, right? That is how it is done in Linux and Android. In Linux, we also > sort on color format, but it is not the same sort as in Windows. In Linux, for > large resolutions, we favour mjpeg over raw formats. Why? Should I implement > that for Windows as well? > > These are the 8 different implementations I found in the codebase: > Great if you can harmonize them then. Yes, when it comes to MediaStreams we now expect a perfect match except that frame rate can be set lower than the max. This is since MediaStreams match constraints with capabilities. However, capture is also exposed through pepper apis that don't care about capabilities (yet at least) USB2 have a limited bandwidth, that is why mjpg is favoured at higher resolutions. 720P at 30fps is pretty much the limit and it have turned out that being on the limit is not so good. But if the frame rate is good enough without mjpeg, other formats preferably yuv planar formats are better since they (should) produce better quality at a lower cost. The implementations used in chrome are all in media/video/capture. The others are from the standalone projects webrtc and libjingle (which are now merged into one project). > * Windows: > const VideoCaptureCapabilityWin& GetBestMatchedFormat( > int requested_width, > int requested_height, > float requested_frame_rate) const; > Prioritize closest height, width, and frame rate in that order. If we still have > a tie, prioritize color format according to the list of precedence: enum > media::VideoPixelFormat. > > * Android: Just try to open the requested format. > > * Linux: Just try to open the requested resolution and frame rate. Choose color > format according to the list of precedence: > VideoCaptureDeviceLinux::GetListOfUsableFourCCs. > > * Mac, media/video/capture/mac/video_capture_device_mac.mm: > GetBestMatchSupportedResolution > Prioritize closest resolution area. Clamp frame rate. Set color format to UYVY. > > * Mac DeckLink, media/video/capture/mac/video_capture_device_decklink_mac.mm: > Prioritize according to L1 distance for resolution and frame rate. > > * third_party/libjingle/source/talk/media/base/videocapturer.cc: > int64 cricket::VideoCapturer::GetFormatDistance(const VideoFormat& desired, > const VideoFormat& supported) > Very complex with penalties and stuff. > > * third_party/libjingle/source/talk/app/webrtc/videosource.cc: > const cricket::VideoFormat& GetBestCaptureFormat( > const std::vector<cricket::VideoFormat>& formats) > Sort on closest resolution area, then frame rate. > > * third_party/webrtc/modules/video_capture/device_info_impl.cc: > int32_t webrtc::videocapturemodule::DeviceInfoImpl::GetBestMatchedCapability( > const char*deviceUniqueIdUTF8, > const VideoCaptureCapability& requested, > VideoCaptureCapability& resulting) > Prioritization is done according to this algorithm: > 1) Height: As big as possible if < requested, otherwise as close to requested as > possible > 2) Width: As big as possible if < requested, otherwise as close to requested as > possible > 3) Framerate: As high as possible if < requested, otherwise as close to > requested as possible > > https://codereview.chromium.org/558623002/diff/220001/media/video/capture/vid... > File media/video/capture/video_capture_types.h (right): > > https://codereview.chromium.org/558623002/diff/220001/media/video/capture/vid... > media/video/capture/video_capture_types.h:101: float > DiffVideoCaptureFormat(const VideoCaptureFormat& lhs, > On 2014/09/24 07:56:29, perkj wrote: > > Is it possible to place this method somewhere else? I suggest > > video_capture_device.h > > This header file is included a lot of different files in content/renderer as > > well. > > Will fix in next upload.
Patchset #8 (id:240001) has been deleted
Patchset #8 (id:260001) has been deleted
perkj: Please take another look. I have restored the original behaviour as you wanted.
https://codereview.chromium.org/558623002/diff/280001/media/video/capture/vid... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/558623002/diff/280001/media/video/capture/vid... media/video/capture/video_capture_device.cc:101: const uint64_t diff_color_format = supported.pixel_format; I am afraid that pixelformat is not really sorted in the order of preference. Can we move YV12 to just below I420 and document in that list that it s sorted. https://codereview.chromium.org/558623002/diff/280001/media/video/capture/vid... media/video/capture/video_capture_device.cc:103: const int resolution_bit_size = 16; nit: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names https://codereview.chromium.org/558623002/diff/280001/media/video/capture/win... File media/video/capture/win/video_capture_device_mf_win.cc (right): https://codereview.chromium.org/558623002/diff/280001/media/video/capture/win... media/video/capture/win/video_capture_device_mf_win.cc:67: kFirstVideoStream, stream_index, type.Receive()))) { indentation
perkj: PTAL. I changed the order of the pixel formats. https://codereview.chromium.org/558623002/diff/280001/media/video/capture/vid... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/558623002/diff/280001/media/video/capture/vid... media/video/capture/video_capture_device.cc:101: const uint64_t diff_color_format = supported.pixel_format; On 2014/10/10 09:17:13, perkj wrote: > I am afraid that pixelformat is not really sorted in the order of preference. > Can we move YV12 to just below I420 and document in that list that it s sorted. Done. https://codereview.chromium.org/558623002/diff/280001/media/video/capture/vid... media/video/capture/video_capture_device.cc:103: const int resolution_bit_size = 16; On 2014/10/10 09:17:13, perkj wrote: > nit: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names These constants are in local scope, so it's ok to use the usual variable naming: "This convention may optionally be used for compile-time constants of local scope; otherwise the usual variable naming rules apply." https://codereview.chromium.org/558623002/diff/280001/media/video/capture/win... File media/video/capture/win/video_capture_device_mf_win.cc (right): https://codereview.chromium.org/558623002/diff/280001/media/video/capture/win... media/video/capture/win/video_capture_device_mf_win.cc:67: kFirstVideoStream, stream_index, type.Receive()))) { On 2014/10/10 09:17:13, perkj wrote: > indentation This is correct according to clang-format already.
lgtm
mcasas: Can you take a look again? I have made some changes since your last lgtm.
https://codereview.chromium.org/558623002/diff/320001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_mac.mm (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/mac... media/video/capture/mac/video_capture_device_mac.mm:101: for (size_t i = 0; i < arraysize(kWellSupportedResolutions); ++i) { nit: range-based for loop...? ;) https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... File media/video/capture/video_capture_device.cc (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... media/video/capture/video_capture_device.cc:101: const uint64_t diff_color_format = supported.pixel_format; s/diff_color_format/diff_pixel_format/g Forgot to subtract from |requested.pixel_format|? https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... media/video/capture/video_capture_device.cc:103: const int resolution_bit_size = 16; s/resolution_bit_size/kResolutionBitSize/ this also applies to l.104-105, but read first l.112 . https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... media/video/capture/video_capture_device.cc:112: uint64_t distance = diff_height; From this point on, this seems to be an obscure linear combination. I don't think the optimization of using << and | is worth it. Besides, you are taken for granted Little Endian, right? Propose: return diff_height * kHeightWeight + diff_width * kWidthWeight + diff_frame_rate * kFrameRateWeight + diff_pixel_format * kPixelFormatWeight; https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... media/video/capture/video_capture_device.h:338: uint64_t DiffVideoCaptureFormat(const VideoCaptureFormat& requested, Why is this in video_capture_device.{h,cc} and not in video_capture_types.{cc,h} ? AFAIK this is a convenience function for manipulating data types defined in the latter files... https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... File media/video/capture/video_capture_types.h (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/vid... media/video/capture/video_capture_types.h:20: enum VideoPixelFormat { You have to update histograms.xml as well https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:15: for (CapabilityList::const_iterator it = capabilities.begin(); Consider using: for (const auto& it : capabilities) https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... File media/video/capture/win/capability_list_win.h (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... media/video/capture/win/capability_list_win.h:18: struct VideoCaptureCapabilityWin { Calling VideoBlaWin to a structure under folder video/capture/win is redundant (not your fault). Specially since the use of it is to create a list that hasn't the "VideoCapture" prefix on it :) So: please consolidate into a single naming. Suggest: CapabilityWin, CapabilityWinList. And then it'll match the file name. Keep the _win in the file name since it's used by .gyp to automatically filter files by platform. https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... media/video/capture/win/capability_list_win.h:27: CapabilityList::const_iterator GetBestMatchedCapability( Why const? https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... File media/video/capture/win/video_capture_device_win.cc (right): https://codereview.chromium.org/558623002/diff/320001/media/video/capture/win... media/video/capture/win/video_capture_device_win.cc:327: // than the capability. I think this comment is superfluous.
Patchset #10 (id:470001) has been deleted
Patchset #10 (id:490001) has been deleted
Patchset #10 (id:530001) has been deleted
Patchset #10 (id:600001) has been deleted
ptal, mcasas: Please take another look. I have moved the Diff-function back to capability_list_win.cc and made it more similar to the original implementation. Also, I have reverted the pixel format order, because it causes some tests to fail. I will fix the pixel format order in a separate CL, because it is unrelated to this CL.
lgtm modulo our comments about std::bind versus base::Bind, unclear which one to prefer now that C++11 is in the picture.
mcasas: I removed std::min_element and std::bind and replaced with a for loop instead. Still lgty?
Couple of comments. Almost there. https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:45: CapabilityWin best_match = capabilities.front(); const CapabilityWin& https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:45: CapabilityWin best_match = capabilities.front(); What if the first capability is bad and none improves its CompareCapability()? Shouldn't we return an empty Capability in that case...? Bah probably this is all paranoia, seems like before (l.117 on the original file) we could end up with an empty |diff_list| and still do a .front.capability() on it :? https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:47: if (CompareCapability(requested, capability, best_match)) { No need for {} in the if.
mcasas: PTAL. https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... File media/video/capture/win/capability_list_win.cc (right): https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:45: CapabilityWin best_match = capabilities.front(); On 2014/10/14 15:17:35, mcasas wrote: > const CapabilityWin& Not possible because I can't reassign a reference. https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:45: CapabilityWin best_match = capabilities.front(); On 2014/10/14 15:17:35, mcasas wrote: > What if the first capability is bad and none improves its CompareCapability()? > Shouldn't we return an empty Capability in that case...? Bah probably this is > all paranoia, seems like before (l.117 on the original file) we could end up > with an empty |diff_list| and still do a .front.capability() on it :? I know the list is non-empty, because I do DCHECK(!capabilities.empty()); This is exactly as the previous behaviour. https://codereview.chromium.org/558623002/diff/640001/media/video/capture/win... media/video/capture/win/capability_list_win.cc:47: if (CompareCapability(requested, capability, best_match)) { On 2014/10/14 15:17:35, mcasas wrote: > No need for {} in the if. Done.
lgtm
The CQ bit was checked by magjed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/558623002/660001
Message was sent while issue was closed.
Committed patchset #12 (id:660001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/04320485ac67d967ead8f4fc267c254293f33ba7 Cr-Commit-Position: refs/heads/master@{#299499} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
