Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7229013:
This is the VideoCaptureDevice implementation for windows. (Closed)
|
Created:
9 years, 6 months ago by Per K Modified:
9 years, 5 months ago Reviewers:
cpu_(ooo_6.6-7.5), tommi (sloooow) - chröme, commit-bot: I haz the power, scherkus (not reviewing) CC:
chromium-reviews, hclam+watch_chromium.org, sjl, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., annacc, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionThis is the VideoCaptureDevice implementation for windows.
DirectShow is used for controlling cameras and receiving video frames.
It implements all COM functions needed to support cameras using DirectShow and don't depend on Microsofts base classes for DirectShow filters.
It supersedes the patch http://codereview.chromium.org/6929064/.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91096
Patch Set 1 #
Total comments: 103
Patch Set 2 : '' #
Total comments: 41
Patch Set 3 : Fixed code review issues found by Scherkus and Tommi. #
Total comments: 12
Patch Set 4 : Fixed reference count issue found by Tommi. #
Total comments: 31
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : Fix crash on some Windows versions when there is no camera available. #Messages
Total messages: 18 (0 generated)
ok, first batch of comments ready :) I know it's a lot but most are minor. http://codereview.chromium.org/7229013/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7229013/diff/1/media/media.gyp#newcode259 media/media.gyp:259: 'video/capture/video_capture_device_dummy.cc', can we instead not include the dummy implementation in the main source list and only add it for platforms that don't need it? http://codereview.chromium.org/7229013/diff/1/media/media.gyp#newcode264 media/media.gyp:264: 'strmiids.lib' Since this is not working, it's OK with me to use #pragma comment(lib) in a _win.cc source file to include this lib. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:9: remove extra empty line. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:22: class MockFrameObserver: public media::VideoCaptureDevice::EventHandler { space before : Can you run gcl lint on the cl and fix all the lint issues? I'll ignore things for now that lint should catch until you upload an update. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:31: virtual void OnError() { for virtual methods Chromium has started to use the OVERRIDE macro/compiler feature. So this should be: virtual void OnError() OVERRIDE { same for other virtual methods. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:34: void OnFrameInfo(const VideoCaptureDevice::Capability& info) { should this be virtual? also add an empty line between functions when they have an implementation. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:42: bool frame_received_; private/protected? http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:56: ASSERT_GT(static_cast<int>(names_.size()), 0); instead of using static_cast<int>(), just do: ASSERT_GT(names_.size(), 0U); Actually, when we're not using the dummy implementation but actually require a camera to be present in order to run the test, I think we should not assert here. The bots are not going to have a camera (at least not all) and there's no need to break the build in that case. Instead, we should log (either vlog(1) or warning level) that the test will not run due to missing hardware. if (names_.empty()) { LOG(WARNING) << "No camera available. " << "This test will be skipped."; skip_test_ = true; <- initialized to false. } and in the tests themselves: if (skip_test_) return; http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:72: EXPECT_TRUE(device == NULL); fyi - I'm sure lint will tell you to use EXPECT_EQ. If you do, you'll need to cast the NULL to the proper type: EXPECT_EQ(device, static_cast<VideoCaptureDevice*>(NULL)); I think it's worth it just to keep lint happy and the code consistent. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:81: EXPECT_CALL(*frame_observer_, OnFrameInfo(640, 480, 30)) is there a chance that allocate succeeds but the frame rate is not what we asked for? On my laptop the Capture720p test succeeds but the frame rate I get is 15 although we're asking for 30. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:90: EXPECT_TRUE(wait_event_.TimedWait(base::TimeDelta::FromMilliseconds(3000))); add a constant at the top of the file for the wait period. If 3000 turns out to be too short it'll be easier to change it. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:120: ASSERT_FALSE(device1.get() == NULL); nit: All the ASSERT_FALSE(ptr == NULL) checks read backwards to me. I look at the condition and I think "he expects the pointer to be NULL". Then later I see that pointer being used and go back to the assert. Only then do I actually notice the ASSERT_*FALSE* bit :) If this is the way you prefer to do the checks, then that's fine but if you don't mind changing them to ASSERT_TRUE(ptr != NULL) then that would imho make them more readable. I guess my reasoning would be that for all conditional statements in C++ (if(), while() do()) there is no if_not(), while_not(), do_not(), so we're accustomed to look at the expression and evaluate whether it's true or not and not false or not. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:22: // Implement from IUnknown. nit: // IUnknown implementation. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:38: int ret = --ref_count_; is the ref_count_ variable only used in order to have a 0 or non-0 return value from AddRef/Release? If so, then you could also just remove it and always return 1 from these methods as the return value should not be relied upon. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:62: return S_OK; should index_ be corrected to point at the end or is it OK for it to point way past the end? Actually, for correctness I would prefer to check if |count| is going to be within the skip-able range and if so, increment index_, if not, move index_ to the end. Otherwise, you might run into strange things if there's a bug that passes -1 (0xffffffff) for count and you end up going backwards. I see that index_ is an int too, which leaves it open to becoming negative in that case. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:84: int index_; suggest size_t instead of int http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:85: int ref_count_; remove? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:105: info->pGraph = owning_graph_; do we need to check if info is NULL and bail if so? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:177: int ret = --ref_count_; remove ref_count_? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:51: AM_MEDIA_TYPE* type = static_cast<AM_MEDIA_TYPE*> (CoTaskMemAlloc( This should be reinterpret_cast. Also, no space between > and ( Same elsewhere where CoTaskMemAlloc is called. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:52: sizeof(AM_MEDIA_TYPE))); indent http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:61: BYTE *format = static_cast<BYTE*> (CoTaskMemAlloc( BYTE* format = ... same as above for cast and space http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:80: *fetched = types_fetched; fix space http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:106: void FreeAllocatedMediaTypes(int allocated, AM_MEDIA_TYPE** types) { should allocated be of type ULONG? (and |i| in the loop as well) http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:115: int ref_count_; remove? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:126: /* Called on an output pin to and establish a use // instead of /* same throughout. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:169: } else { nit: no need for an else. just do as above. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:175: if (!connected_pin_) { nit: remove braces http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:182: STDMETHODIMP PinBase::QueryPinInfo(PIN_INFO* pInfo) { remove hungarian http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:185: if (owner_) { nit: remove braces http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:280: int ret = owner_->Release(); this looks like it could be a circular reference. Does the owner hold a reference to this object too? If so, how do you guarantee that the objects are fully released? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.h:37: virtual HRESULT STDMETHODCALLTYPE Receive(IMediaSample *pSample) = 0; follow chromium style (variable naming and position of *) and use STDMETHOD(). I.e. STDMETHOD(Receive)(IMediaSample* sample) = 0; same for all other COM methods. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.h:99: IPin* connected_pin_; ScopedComPtr? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.h:100: IBaseFilter* owner_; ScopedComPtr? if not, document why. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... File media/video/capture/win/sink_input_pin_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:25: if (media_type->cbFormat < sizeof(VIDEOINFOHEADER)) { nit: no need for braces http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:46: case 0: { enum values for the index? Instead of 0,1,2 maybe INDEX_I420, INDEX_YUY2, INDEX_RGB? unless 0,1,2 is clear to everyone that calls the method. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:97: reinterpret_cast<VIDEOINFOHEADER*>(media_type->pbFormat); indent http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:119: ::Sleep(60); // Workaround for bad driver. is this a driver that's always installed or a 3rd party driver? Also, why 60 and not 10 or 100? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:121: CoTaskMemFree((PVOID)mt->pbFormat); reinterpret_cast<void*>() ... actually, do you need a cast at all? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:126: // pUnk should not be used. in that case, maybe add a NOTREACHED()? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:134: void DeleteMediaType(AM_MEDIA_TYPE *pmt) { fix * http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:211: if ((wcsstr(str_ptr, L"(VFW)") == NULL) && add a note on what the string might look like if we see this and why we ignore it? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:212: (_wcsnicmp(str_ptr, kGoogleCameraAdapter, use strncasecmp or perhaps LowerCaseEqualsASCII (string_util.h) http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:319: return; Should SetErrorState be called? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:386: hr = graph_builder_->ConnectDirect(output_capture_pin_, input_sink_pin_, When we get here, it is possible that |hr| indicates an error if mpjpg_filter_.CreateInstance failed. Is it OK to overwrite |hr| without calling SetErrorState? capability.color might still be kMJPEG but mjpg_filter_ would be NULL. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:412: return; SetErrorState? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:420: state_ = kAllocated; bug? state_ will already be kAllocated at this point, so should this be kCapturing? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:433: state_ = kAllocated; is it OK to change the state even though Stop() failed? maybe Stop() was called from the wrong thread and failed because of that and if so, the state_ shouldn't be changed? :) just pondering... http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:480: AM_MEDIA_TYPE* pmt = NULL; no hungarian http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:593: diff_list.sort(CompareHeight); should this be diff_list.sort(&CompareHeight)? (and below) Even if this compiles on Windows, it might break on clang. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... File media/video/capture/win/video_capture_device_win.h (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.h:50: kIdle, // The device driver is opened but camera is not in use. Chromium style isn't to use kName names for enum values. Instead use IDLE, ALLOCATED, CAPTURING, ERROR or perhaps STATE_IDLE, STATE_ALLOCATED, etc.
drive-by style-ish nits I'll leave it up to tommi for reviewing the actual functionality + COM bits :) http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... media/video/capture/win/filter_base_win.cc:12: class PinEnumerator : public IEnumPins, nit: style should be same as initializer list (drop to next line, 4 space indent, etc.) http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... File media/video/capture/win/filter_base_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... media/video/capture/win/filter_base_win.h:21: class FilterBase : public IBaseFilter, nit: style should be same as initializer list (drop to next line, 4 space indent, etc.) http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... media/video/capture/win/pin_base_win.cc:12: class TypeEnumerator : public IEnumMediaTypes, nit: style should be same as initializer list (drop to next line, 4 space indent, etc.) http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... media/video/capture/win/pin_base_win.h:20: class PinBase : public IPin, nit: style should be same as initializer list (drop to next line, 4 space indent, etc.) http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_filter_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.cc:20: SinkFilter::SinkFilterObserver::~SinkFilterObserver() { nit: collapse into {} http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.cc:28: SinkFilter::~SinkFilter() { nit: collapse into {} http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_filter_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.h:24: static const REFERENCE_TIME kSecondsToReferenceTime = 10000000; move to .cc http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.h:31: class SinkFilterObserver { move to separate .h http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_input_pin_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.cc:21: SinkInputPin::~SinkInputPin() { --> {} http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.cc:97: reinterpret_cast<VIDEOINFOHEADER*>(media_type->pbFormat); indent by 2 more spaces http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.cc:119: ::Sleep(60); // Workaround for bad driver. woah! why do we need this? http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_input_pin_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.h:21: explicit SinkInputPin(IBaseFilter* filter, >1 params -> no explicit http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.h:40: SinkFilter::SinkFilterObserver* const observer_; no need for const pointers http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:51: for (size_t i = 0; i < arraysize(kPropertyNames) && nit: I'd drop 2nd clause to next line instead of indenting like this http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:208: // ignore all VFW drivers and the special Google Camera Adapter capitalize + end w/ period what is the Google Camera Adapter? http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:210: static const wchar_t kGoogleCameraAdapter[] = L"Google Camera Adapter"; move to top of file http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:405: observer_->OnFrameInfo(used_capability); inline the function call here? http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.h:50: kIdle, // The device driver is opened but camera is not in use. nit: chromium typically uses kCamelCaseStyle for constants, but still sticks to ENUM_STYLE for enums http://dev.chromium.org/developers/coding-style
oh don't forget BUG= and TEST= in your CL description!
Fixed review issues. / Per http://codereview.chromium.org/7229013/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7229013/diff/1/media/media.gyp#newcode259 media/media.gyp:259: 'video/capture/video_capture_device_dummy.cc', On 2011/06/23 14:05:56, tommi wrote: > can we instead not include the dummy implementation in the main source list and > only add it for platforms that don't need it? Isn't it better this way? This way we will be able to build on all platforms regardless if a real video capture device implementation exist. http://codereview.chromium.org/7229013/diff/1/media/media.gyp#newcode264 media/media.gyp:264: 'strmiids.lib' On 2011/06/23 14:05:56, tommi wrote: > Since this is not working, it's OK with me to use #pragma comment(lib) in a > _win.cc source file to include this lib. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:9: On 2011/06/23 14:05:56, tommi wrote: > remove extra empty line. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:22: class MockFrameObserver: public media::VideoCaptureDevice::EventHandler { On 2011/06/23 14:05:56, tommi wrote: > space before : > > Can you run gcl lint on the cl and fix all the lint issues? > I'll ignore things for now that lint should catch until you upload an update. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:31: virtual void OnError() { On 2011/06/23 14:05:56, tommi wrote: > for virtual methods Chromium has started to use the OVERRIDE macro/compiler > feature. So this should be: > virtual void OnError() OVERRIDE { > > same for other virtual methods. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:34: void OnFrameInfo(const VideoCaptureDevice::Capability& info) { On 2011/06/23 14:05:56, tommi wrote: > should this be virtual? > also add an empty line between functions when they have an implementation. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:42: bool frame_received_; On 2011/06/23 14:05:56, tommi wrote: > private/protected? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:56: ASSERT_GT(static_cast<int>(names_.size()), 0); Done - kind of. On 2011/06/23 14:05:56, tommi wrote: > instead of using static_cast<int>(), just do: > ASSERT_GT(names_.size(), 0U); > > Actually, when we're not using the dummy implementation but actually require a > camera to be present in order to run the test, I think we should not assert > here. > The bots are not going to have a camera (at least not all) and there's no need > to break the build in that case. Instead, we should log (either vlog(1) or > warning level) that the test will not run due to missing hardware. > > if (names_.empty()) { > LOG(WARNING) << "No camera available. " > << "This test will be skipped."; > skip_test_ = true; <- initialized to false. > } > > and in the tests themselves: > > if (skip_test_) > return; http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:72: EXPECT_TRUE(device == NULL); Lint don't complain about this. On 2011/06/23 14:05:56, tommi wrote: > fyi - I'm sure lint will tell you to use EXPECT_EQ. If you do, you'll need to > cast the NULL to the proper type: > > EXPECT_EQ(device, static_cast<VideoCaptureDevice*>(NULL)); > > I think it's worth it just to keep lint happy and the code consistent. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:81: EXPECT_CALL(*frame_observer_, OnFrameInfo(640, 480, 30)) On 2011/06/23 14:05:56, tommi wrote: > is there a chance that allocate succeeds but the frame rate is not what we asked > for? On my laptop the Capture720p test succeeds but the frame rate I get is 15 > although we're asking for 30. Almost all cameras can do VGA 30fps. But whenever a resolution is not supported the callback will give you the nearest possible resolution. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:90: EXPECT_TRUE(wait_event_.TimedWait(base::TimeDelta::FromMilliseconds(3000))); On 2011/06/23 14:05:56, tommi wrote: > add a constant at the top of the file for the wait period. If 3000 turns out to > be too short it'll be easier to change it. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/video_captu... media/video/capture/video_capture_device_unittest.cc:120: ASSERT_FALSE(device1.get() == NULL); On 2011/06/23 14:05:56, tommi wrote: > nit: All the ASSERT_FALSE(ptr == NULL) checks read backwards to me. I look at > the condition and I think "he expects the pointer to be NULL". Then later I see > that pointer being used and go back to the assert. Only then do I actually > notice the ASSERT_*FALSE* bit :) > If this is the way you prefer to do the checks, then that's fine but if you > don't mind changing them to ASSERT_TRUE(ptr != NULL) then that would imho make > them more readable. > > I guess my reasoning would be that for all conditional statements in C++ (if(), > while() do()) there is no if_not(), while_not(), do_not(), so we're accustomed > to look at the expression and evaluate whether it's true or not and not false or > not. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:22: // Implement from IUnknown. On 2011/06/23 14:05:56, tommi wrote: > nit: // IUnknown implementation. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:38: int ret = --ref_count_; On 2011/06/23 14:05:56, tommi wrote: > is the ref_count_ variable only used in order to have a 0 or non-0 return value > from AddRef/Release? > If so, then you could also just remove it and always return 1 from these methods > as the return value should not be relied upon. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:62: return S_OK; Changed to if(index_ >= 0) { if (filter_->NoOfPins() < index_) return S_OK; } index_ = 0; return S_FALSE; S_FALSE means Skipped past the end of the sequence. On 2011/06/23 14:05:56, tommi wrote: > should index_ be corrected to point at the end or is it OK for it to point way > past the end? > Actually, for correctness I would prefer to check if |count| is going to be > within the skip-able range and if so, increment index_, if not, move index_ to > the end. > Otherwise, you might run into strange things if there's a bug that passes -1 > (0xffffffff) for count and you end up going backwards. I see that index_ is an > int too, which leaves it open to becoming negative in that case. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:84: int index_; On 2011/06/23 14:05:56, tommi wrote: > suggest size_t instead of int Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:85: int ref_count_; On 2011/06/23 14:05:56, tommi wrote: > remove? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:105: info->pGraph = owning_graph_; On 2011/06/23 14:05:56, tommi wrote: > do we need to check if info is NULL and bail if so? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/filter_... media/video/capture/win/filter_base_win.cc:177: int ret = --ref_count_; On 2011/06/23 14:05:56, tommi wrote: > remove ref_count_? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:51: AM_MEDIA_TYPE* type = static_cast<AM_MEDIA_TYPE*> (CoTaskMemAlloc( On 2011/06/23 14:05:56, tommi wrote: > This should be reinterpret_cast. > Also, no space between > and ( > > Same elsewhere where CoTaskMemAlloc is called. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:52: sizeof(AM_MEDIA_TYPE))); On 2011/06/23 14:05:56, tommi wrote: > indent Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:61: BYTE *format = static_cast<BYTE*> (CoTaskMemAlloc( On 2011/06/23 14:05:56, tommi wrote: > BYTE* format = ... > same as above for cast and space Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:80: *fetched = types_fetched; On 2011/06/23 14:05:56, tommi wrote: > fix space Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:106: void FreeAllocatedMediaTypes(int allocated, AM_MEDIA_TYPE** types) { On 2011/06/23 14:05:56, tommi wrote: > should allocated be of type ULONG? (and |i| in the loop as well) Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:115: int ref_count_; On 2011/06/23 14:05:56, tommi wrote: > remove? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:126: /* Called on an output pin to and establish a On 2011/06/23 14:05:56, tommi wrote: > use // instead of /* > same throughout. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:169: } else { On 2011/06/23 14:05:56, tommi wrote: > nit: no need for an else. just do as above. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:175: if (!connected_pin_) { On 2011/06/23 14:05:56, tommi wrote: > nit: remove braces Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:182: STDMETHODIMP PinBase::QueryPinInfo(PIN_INFO* pInfo) { On 2011/06/23 14:05:56, tommi wrote: > remove hungarian Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:185: if (owner_) { On 2011/06/23 14:05:56, tommi wrote: > nit: remove braces Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.cc:280: int ret = owner_->Release(); This is to make sure the owner is not deleted before the pin. But the owner does not reference count the pin and the pin itself does not reference count the owner. On 2011/06/23 14:05:56, tommi wrote: > this looks like it could be a circular reference. > Does the owner hold a reference to this object too? > If so, how do you guarantee that the objects are fully released? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.h:37: virtual HRESULT STDMETHODCALLTYPE Receive(IMediaSample *pSample) = 0; On 2011/06/23 14:05:56, tommi wrote: > follow chromium style (variable naming and position of *) and use STDMETHOD(). > I.e. > > STDMETHOD(Receive)(IMediaSample* sample) = 0; > > same for all other COM methods. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.h:99: IPin* connected_pin_; On 2011/06/23 14:05:56, tommi wrote: > ScopedComPtr? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/pin_bas... media/video/capture/win/pin_base_win.h:100: IBaseFilter* owner_; On 2011/06/23 14:05:56, tommi wrote: > ScopedComPtr? if not, document why. Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... File media/video/capture/win/sink_input_pin_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:25: if (media_type->cbFormat < sizeof(VIDEOINFOHEADER)) { On 2011/06/23 14:05:56, tommi wrote: > nit: no need for braces Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:46: case 0: { It called from TypeEnumerator::Next where index is used. I think int is good but can be convinced otherwise. On 2011/06/23 14:05:56, tommi wrote: > enum values for the index? Instead of 0,1,2 maybe INDEX_I420, INDEX_YUY2, > INDEX_RGB? > unless 0,1,2 is clear to everyone that calls the method. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:97: reinterpret_cast<VIDEOINFOHEADER*>(media_type->pbFormat); On 2011/06/23 14:05:56, tommi wrote: > indent Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/sink_in... media/video/capture/win/sink_input_pin_win.cc:119: ::Sleep(60); // Workaround for bad driver. This is an old fix that we unfortunately are not sure of the origin. Best guess is that this was a fix for an old HP laptop. I will remove this for now. On 2011/06/23 14:05:56, tommi wrote: > is this a driver that's always installed or a 3rd party driver? Also, why 60 > and not 10 or 100? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:121: CoTaskMemFree((PVOID)mt->pbFormat); On 2011/06/23 14:05:56, tommi wrote: > reinterpret_cast<void*>() > ... actually, do you need a cast at all? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:126: // pUnk should not be used. On 2011/06/23 14:05:56, tommi wrote: > in that case, maybe add a NOTREACHED()? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:134: void DeleteMediaType(AM_MEDIA_TYPE *pmt) { On 2011/06/23 14:05:56, tommi wrote: > fix * Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:211: if ((wcsstr(str_ptr, L"(VFW)") == NULL) && On 2011/06/23 14:05:56, tommi wrote: > add a note on what the string might look like if we see this and why we ignore > it? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:212: (_wcsnicmp(str_ptr, kGoogleCameraAdapter, On 2011/06/23 14:05:56, tommi wrote: > use strncasecmp or perhaps LowerCaseEqualsASCII (string_util.h) Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:319: return; Nope. This means that the VideoCaptureDevice have been used in the wrong way or we already are in error state. On 2011/06/23 14:05:56, tommi wrote: > Should SetErrorState be called? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:386: hr = graph_builder_->ConnectDirect(output_capture_pin_, input_sink_pin_, The idea is that we would still try to Connect the filter. It we are able to connect that is fine but most likely it will fail since our base filter don't support mjpg. That will lead to that SetErrorstate is called. On 2011/06/23 14:05:56, tommi wrote: > When we get here, it is possible that |hr| indicates an error if > mpjpg_filter_.CreateInstance failed. Is it OK to overwrite |hr| without calling > SetErrorState? > > capability.color might still be kMJPEG but mjpg_filter_ would be NULL. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:412: return; Nope. This means that the VideoCaptureDevice have been used in the wrong way or we already are in error state. On 2011/06/23 14:05:56, tommi wrote: > SetErrorState? http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:420: state_ = kAllocated; On 2011/06/23 14:05:56, tommi wrote: > bug? > state_ will already be kAllocated at this point, so should this be kCapturing? Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:433: state_ = kAllocated; I think you mean in DeAllocate? I just changed so it returns. On 2011/06/23 14:05:56, tommi wrote: > is it OK to change the state even though Stop() failed? > maybe Stop() was called from the wrong thread and failed because of that and if > so, the state_ shouldn't be changed? :) > just pondering... http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:480: AM_MEDIA_TYPE* pmt = NULL; On 2011/06/23 14:05:56, tommi wrote: > no hungarian Done. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.cc:593: diff_list.sort(CompareHeight); Interesting. Both seems to work. According to the example on http://www.cplusplus.com/reference/stl/list/sort/ it is correct without &. Anyway I changed to your suggestion. On 2011/06/23 14:05:56, tommi wrote: > should this be > diff_list.sort(&CompareHeight)? > (and below) > > Even if this compiles on Windows, it might break on clang. http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... File media/video/capture/win/video_capture_device_win.h (right): http://codereview.chromium.org/7229013/diff/1/media/video/capture/win/video_c... media/video/capture/win/video_capture_device_win.h:50: kIdle, // The device driver is opened but camera is not in use. Is it mixed? In the media namespace kNames are used heavily and I have also used it in the linux implementation of VideoCapture. Would it be ok to keep it this style? On 2011/06/23 14:05:56, tommi wrote: > Chromium style isn't to use kName names for enum values. > Instead use IDLE, ALLOCATED, CAPTURING, ERROR or perhaps STATE_IDLE, > STATE_ALLOCATED, etc. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... media/video/capture/win/filter_base_win.cc:12: class PinEnumerator : public IEnumPins, On 2011/06/24 20:15:26, scherkus wrote: > nit: style should be same as initializer list (drop to next line, 4 space > indent, etc.) Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... File media/video/capture/win/filter_base_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/filt... media/video/capture/win/filter_base_win.h:21: class FilterBase : public IBaseFilter, On 2011/06/24 20:15:26, scherkus wrote: > nit: style should be same as initializer list (drop to next line, 4 space > indent, etc.) Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... media/video/capture/win/pin_base_win.cc:12: class TypeEnumerator : public IEnumMediaTypes, On 2011/06/24 20:15:26, scherkus wrote: > nit: style should be same as initializer list (drop to next line, 4 space > indent, etc.) Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... media/video/capture/win/pin_base_win.h:20: class PinBase : public IPin, On 2011/06/24 20:15:26, scherkus wrote: > nit: style should be same as initializer list (drop to next line, 4 space > indent, etc.) Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_filter_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.cc:20: SinkFilter::SinkFilterObserver::~SinkFilterObserver() { On 2011/06/24 20:15:26, scherkus wrote: > nit: collapse into {} Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.cc:28: SinkFilter::~SinkFilter() { On 2011/06/24 20:15:26, scherkus wrote: > nit: collapse into {} Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_filter_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.h:24: static const REFERENCE_TIME kSecondsToReferenceTime = 10000000; On 2011/06/24 20:15:26, scherkus wrote: > move to .cc Moved to header file for sink_input_pin.h. This is used in two cc files. Both video_capture_device_win.cc and sink_input_pin.cc. Is this ok? http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_filter_win.h:31: class SinkFilterObserver { On 2011/06/24 20:15:26, scherkus wrote: > move to separate .h Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_input_pin_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.cc:21: SinkInputPin::~SinkInputPin() { On 2011/06/24 20:15:26, scherkus wrote: > --> {} Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.cc:97: reinterpret_cast<VIDEOINFOHEADER*>(media_type->pbFormat); On 2011/06/24 20:15:26, scherkus wrote: > indent by 2 more spaces Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.cc:119: ::Sleep(60); // Workaround for bad driver. Now removed. Please see comment to patchset 1. Basically this fixed an issue a long time ago and we have not dared to remove it since we are no longer shore what it fixed. (Even before my time I think). On 2011/06/24 20:15:26, scherkus wrote: > woah! > > why do we need this? http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... File media/video/capture/win/sink_input_pin_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.h:21: explicit SinkInputPin(IBaseFilter* filter, On 2011/06/24 20:15:26, scherkus wrote: > >1 params -> no explicit Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/sink... media/video/capture/win/sink_input_pin_win.h:40: SinkFilter::SinkFilterObserver* const observer_; On 2011/06/24 20:15:26, scherkus wrote: > no need for const pointers Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:51: for (size_t i = 0; i < arraysize(kPropertyNames) && On 2011/06/24 20:15:26, scherkus wrote: > nit: I'd drop 2nd clause to next line instead of indenting like this Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:208: // ignore all VFW drivers and the special Google Camera Adapter Added comment in file. It is fake DirectShow camera that GTalk install in order to access cameras. What the reason is I don't know. But we don't want people to be able to choose this as a camera. On 2011/06/24 20:15:26, scherkus wrote: > capitalize + end w/ period > > what is the Google Camera Adapter? http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:210: static const wchar_t kGoogleCameraAdapter[] = L"Google Camera Adapter"; On 2011/06/24 20:15:26, scherkus wrote: > move to top of file Done. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:405: observer_->OnFrameInfo(used_capability); On 2011/06/24 20:15:26, scherkus wrote: > inline the function call here? Can you please elaborate? observer is the VideoCaptuerDeviceObserver implemented by VideoCaptureController. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.h:50: kIdle, // The device driver is opened but camera is not in use. Ok- Tommy commented on this as well. But "for consistancy" I would like to keep this so it is the same as in VideoCaptureDeviceLinux. On 2011/06/24 20:15:26, scherkus wrote: > nit: chromium typically uses kCamelCaseStyle for constants, but still sticks to > ENUM_STYLE for enums > > http://dev.chromium.org/developers/coding-style
Great. Only a couple more minor things. http://codereview.chromium.org/7229013/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7229013/diff/1/media/media.gyp#newcode259 media/media.gyp:259: 'video/capture/video_capture_device_dummy.cc', On 2011/06/27 11:47:50, Per K wrote: > On 2011/06/23 14:05:56, tommi wrote: > > can we instead not include the dummy implementation in the main source list > and > > only add it for platforms that don't need it? > > Isn't it better this way? This way we will be able to build on all platforms > regardless if a real video capture device implementation exist. sounds good. http://codereview.chromium.org/7229013/diff/7002/media/video/capture/video_ca... File media/video/capture/video_capture_device_unittest.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/video_ca... media/video/capture/video_capture_device_unittest.cc:74: if (!names_.size()) { use if (names_.empty()) http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/pin_... media/video/capture/win/pin_base_win.h:37: virtual HRESULT STDMETHODCALLTYPE Receive(IMediaSample *pSample) = 0; fix all "virtual HRESULT STDMETHODCALLTYPE" to use STDMETHOD() http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.h:50: kIdle, // The device driver is opened but camera is not in use. (I just learned this) According to the style guide Chromium has switched over to kCamelCaps style for enums. So since this is getting some traction in the media folder I think we should go with the new hip and cool style. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:62: if (index_ >= 0) { index_ is of type size_t (unsigned), so this will always be true. nit: it is still possible to go backwards since you already add count to index_ without checking |count| first. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:51: sizeof(AM_MEDIA_TYPE))); indent http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:270: return owner_->AddRef(); as discussed in person this is now done a bit differently. Thanks for bearing with me :) http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.h:38: STDMETHOD(Receive)(IMediaSample* pSample) = 0; pSample -> sample http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.h:40: STDMETHOD(Connect)(IPin* pReceivePin, const AM_MEDIA_TYPE* pmt); fix variable names here and below. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:221: wcslen(str_ptr) < name_length || nit: use lstrlenW instead of wcslen since this is a windows only file. The difference is that some versions of wcslen have been known to crash and it's an extra crt dependency. lstrlenW on the other hand is implemented in kernel32 and is pretty much guaranteed to be "hot" and always handles NULLs properly ;)
In general it look great and I don't have the IQ to understand the logic here. So lgtm on my side with these nits: http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:36: return 1; it is a little bit puzzling returning 1, in here and elsewhere in this code. While it is true that nobody normally should care about AddRef and Release return values, returning always 1 means you forfeit some avenues for debugging. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:170: } nit: this method is structured in a strange way. Might I suggest a single addref()? } else { return E_NOINTERFACE; } AddRef(); return S_OK; } http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:146: current_media_type_ = current_media_type_; nit: The negative logic is more readable, prefer it. if (!IsMediaTypeValid(mt)) return VFW_E_TYPE_NOT_ACCEPTED; Like line 130. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:156: connected_pin_.Release(); same nit as 145 and elsewhere. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:179: DCHECK(info); In general I avoid this pattern DCHECK(foo); foo->bar = x; or DCHECK(foo); foo->Bar(); As you can imagine foo being null will cause a crash anyway in basically the same place. Now if the dcheck had some other piece of information beyond the stacktrace then I would be more amenable to it. Besides is not even applied uniformly, like see line 190. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:264: AddRef(); same http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... File media/video/capture/win/sink_input_pin_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... media/video/capture/win/sink_input_pin_win.cc:94: if (format_type == FORMAT_VideoInfo) { same nit about negative logic. if (format_type != blah) return false; http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:174: static const char kGoogleCameraAdapter[] = "google camera adapter"; just fyi, this filtering is also done in libjingle, one day we should unify. Not today though :) http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:217: const wchar_t* str_ptr = V_BSTR(&name); do we need to do this inside the loop? http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:242: if (!self || !self->Init()) { if (self && self->Init()) return self;
tiny nits but LGTM will leave up to tommi for final LGTM http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.cc:405: observer_->OnFrameInfo(used_capability); On 2011/06/27 11:47:50, Per K wrote: > On 2011/06/24 20:15:26, scherkus wrote: > > inline the function call here? > > Can you please elaborate? observer is the VideoCaptuerDeviceObserver implemented > by VideoCaptureController. oh I meant because you're not using the variable you could write: observer_->OnFrameInfo(sink_filter_->ResultingCapability()); http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_win.h (right): http://codereview.chromium.org/7229013/diff/7002/media/video/capture/win/vide... media/video/capture/win/video_capture_device_win.h:50: kIdle, // The device driver is opened but camera is not in use. On 2011/06/27 13:20:37, tommi wrote: > (I just learned this) According to the style guide Chromium has switched over to > kCamelCaps style for enums. So since this is getting some traction in the media > folder I think we should go with the new hip and cool style. ok! http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:107: DCHECK(info); nit: these types of DCHECKS (where we immediately dereference the ptr) are useless see: http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.h:68: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.h:21: class PinBase : public IPin, it: style should be same as initializer list (drop to next line, 4 space indent, etc.) http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... File media/video/capture/win/sink_filter_observer_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... media/video/capture/win/sink_filter_observer_win.h:12: virtual void FrameReceived(const uint8* buffer, int length) = 0; docs please http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... File media/video/capture/win/sink_input_pin_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... media/video/capture/win/sink_input_pin_win.h:18: const REFERENCE_TIME kSecondsToReferenceTime = 10000000; can you move the actual = 10000000; part to .cc file?
http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:36: return 1; On 2011/06/27 18:10:27, cpu wrote: > it is a little bit puzzling returning 1, in here and elsewhere in this code. > While it is true that nobody normally should care about AddRef and Release > return values, returning always 1 means you forfeit some avenues for debugging. I absolutely agree. I asked Per to do it this way however because the reference counting is already being done by base::RefCounted<> and I don't want to start a pattern of having two reference counters in COM classes just because the RefCounted implementation returns void. Imho it would be useful to change RefCounted's AddRef/Release to return the reference number but I'm sure there was a bike shed discussion on that issue back in the day.
Updated to head rev. Fixed code review issues found by scherkus and cpu. Cheers Per http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:62: if (index_ >= 0) { On 2011/06/27 13:20:37, tommi wrote: > index_ is of type size_t (unsigned), so this will always be true. > > nit: it is still possible to go backwards since you already add count to index_ > without checking |count| first. Done. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:51: sizeof(AM_MEDIA_TYPE))); On 2011/06/27 13:20:37, tommi wrote: > indent Done. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:270: return owner_->AddRef(); On 2011/06/27 13:20:37, tommi wrote: > as discussed in person this is now done a bit differently. Thanks for bearing > with me :) Done. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.h:38: STDMETHOD(Receive)(IMediaSample* pSample) = 0; On 2011/06/27 13:20:37, tommi wrote: > pSample -> sample Done. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/pin... media/video/capture/win/pin_base_win.h:40: STDMETHOD(Connect)(IPin* pReceivePin, const AM_MEDIA_TYPE* pmt); On 2011/06/27 13:20:37, tommi wrote: > fix variable names here and below. Done. http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/19003/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:221: wcslen(str_ptr) < name_length || On 2011/06/27 13:20:37, tommi wrote: > nit: use lstrlenW instead of wcslen since this is a windows only file. The > difference is that some versions of wcslen have been known to crash and it's an > extra crt dependency. lstrlenW on the other hand is implemented in kernel32 and > is pretty much guaranteed to be "hot" and always handles NULLs properly ;) Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:107: DCHECK(info); On 2011/06/27 19:24:01, scherkus wrote: > nit: these types of DCHECKS (where we immediately dereference the ptr) are > useless > > see: > http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE... Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:170: } Yes -thanks. On 2011/06/27 18:10:27, cpu wrote: > nit: this method is structured in a strange way. Might I suggest a single > addref()? > > } else { > return E_NOINTERFACE; > } > > AddRef(); > return S_OK; > } http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.h:68: }; On 2011/06/27 19:24:01, scherkus wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:146: current_media_type_ = current_media_type_; On 2011/06/27 18:10:27, cpu wrote: > nit: The negative logic is more readable, prefer it. > > if (!IsMediaTypeValid(mt)) > return VFW_E_TYPE_NOT_ACCEPTED; > > Like line 130. Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:156: connected_pin_.Release(); On 2011/06/27 18:10:27, cpu wrote: > same nit as 145 and elsewhere. Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:179: DCHECK(info); On 2011/06/27 18:10:27, cpu wrote: > In general I avoid this pattern > > DCHECK(foo); > foo->bar = x; > > or > > DCHECK(foo); > foo->Bar(); > > As you can imagine foo being null will cause a crash anyway in basically the > same place. Now if the dcheck had some other piece of information beyond the > stacktrace then I would be more amenable to it. > > Besides is not even applied uniformly, like see line 190. Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.cc:264: AddRef(); On 2011/06/27 18:10:27, cpu wrote: > same Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... File media/video/capture/win/pin_base_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/pin... media/video/capture/win/pin_base_win.h:21: class PinBase : public IPin, On 2011/06/27 19:24:01, scherkus wrote: > it: style should be same as initializer list (drop to next line, 4 space > indent, etc.) Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... File media/video/capture/win/sink_filter_observer_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... media/video/capture/win/sink_filter_observer_win.h:12: virtual void FrameReceived(const uint8* buffer, int length) = 0; On 2011/06/27 19:24:01, scherkus wrote: > docs please Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... File media/video/capture/win/sink_input_pin_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... media/video/capture/win/sink_input_pin_win.cc:94: if (format_type == FORMAT_VideoInfo) { On 2011/06/27 18:10:27, cpu wrote: > same nit about negative logic. > if (format_type != blah) > return false; Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... File media/video/capture/win/sink_input_pin_win.h (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/sin... media/video/capture/win/sink_input_pin_win.h:18: const REFERENCE_TIME kSecondsToReferenceTime = 10000000; On 2011/06/27 19:24:01, scherkus wrote: > can you move the actual = 10000000; part to .cc file? Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... File media/video/capture/win/video_capture_device_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:174: static const char kGoogleCameraAdapter[] = "google camera adapter"; On 2011/06/27 18:10:27, cpu wrote: > just fyi, this filtering is also done in libjingle, one day we should unify. Not > today though :) Done. http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:217: const wchar_t* str_ptr = V_BSTR(&name); ? Yes- device_names is a list of all available video capture devices. On 2011/06/27 18:10:27, cpu wrote: > do we need to do this inside the loop? http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/vid... media/video/capture/win/video_capture_device_win.cc:242: if (!self || !self->Init()) { On 2011/06/27 18:10:27, cpu wrote: > if (self && self->Init()) > return self; Done.
http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... File media/video/capture/win/filter_base_win.cc (right): http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... media/video/capture/win/filter_base_win.cc:36: return 1; On 2011/06/28 08:57:23, tommi wrote: > On 2011/06/27 18:10:27, cpu wrote: > > it is a little bit puzzling returning 1, in here and elsewhere in this code. > > While it is true that nobody normally should care about AddRef and Release > > return values, returning always 1 means you forfeit some avenues for > debugging. > > I absolutely agree. I asked Per to do it this way however because the reference > counting is already being done by base::RefCounted<> and I don't want to start a > pattern of having two reference counters in COM classes just because the > RefCounted implementation returns void. Imho it would be useful to change > RefCounted's AddRef/Release to return the reference number but I'm sure there > was a bike shed discussion on that issue back in the day. Seeing as we duplicate the impl of AddRef/Release in this + other classes, would it help to define a helper base class that implements it w/ the return 1 and a comment describing why? Something like: // We need to do this because of blah blah blah template <class T> class DShowRefCounted : public base::RefCounted<T> { public: STDMETHOD_(ULONG, AddRef)() { base::RefCounted<T>::AddRef(); return 1; } STDMETHOD_(ULONG, Release)() { base::RefCounted<T>::Release(); return 1; } } http://codereview.chromium.org/7229013/diff/22002/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7229013/diff/22002/media/media.gyp#newcode18 media/media.gyp:18: '../build/temp_gyp/googleurl.gyp:googleurl', ????? media should not depend on googleurl
On 2011/06/28 20:38:41, scherkus wrote: > http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... > File media/video/capture/win/filter_base_win.cc (right): > > http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... > media/video/capture/win/filter_base_win.cc:36: return 1; > On 2011/06/28 08:57:23, tommi wrote: > > On 2011/06/27 18:10:27, cpu wrote: > > > it is a little bit puzzling returning 1, in here and elsewhere in this code. > > > While it is true that nobody normally should care about AddRef and Release > > > return values, returning always 1 means you forfeit some avenues for > > debugging. > > > > I absolutely agree. I asked Per to do it this way however because the > reference > > counting is already being done by base::RefCounted<> and I don't want to start > a > > pattern of having two reference counters in COM classes just because the > > RefCounted implementation returns void. Imho it would be useful to change > > RefCounted's AddRef/Release to return the reference number but I'm sure there > > was a bike shed discussion on that issue back in the day. > > Seeing as we duplicate the impl of AddRef/Release in this + other classes, would > it help to define a helper base class that implements it w/ the return 1 and a > comment describing why? > > Something like: > > // We need to do this because of blah blah blah > template <class T> > class DShowRefCounted : public base::RefCounted<T> { > public: > STDMETHOD_(ULONG, AddRef)() { > base::RefCounted<T>::AddRef(); > return 1; > } > > STDMETHOD_(ULONG, Release)() { > base::RefCounted<T>::Release(); > return 1; > } > } > I discussed this today with tommi. The tricky part is that PinBase inherits from two different I-classes that both inherits from IUnknown. tommi have a suggested solution but it got to complicated for this patch and we therefore suggest a generic COM refcount classes is added among the other COM base classes and this patch is later changed to use it. > http://codereview.chromium.org/7229013/diff/22002/media/media.gyp > File media/media.gyp (right): > > http://codereview.chromium.org/7229013/diff/22002/media/media.gyp#newcode18 > media/media.gyp:18: '../build/temp_gyp/googleurl.gyp:googleurl', > ????? > > media should not depend on googleurl
http://codereview.chromium.org/7229013/diff/22002/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/7229013/diff/22002/media/media.gyp#newcode18 media/media.gyp:18: '../build/temp_gyp/googleurl.gyp:googleurl', ????? This is not part of this patch. It was added by alcolwell in rev 79185 it seems like. On 2011/06/28 20:38:41, scherkus wrote: > ????? > > media should not depend on googleurl
On 2011/06/29 13:33:29, Per K wrote: > On 2011/06/28 20:38:41, scherkus wrote: > > > http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... > > File media/video/capture/win/filter_base_win.cc (right): > > > > > http://codereview.chromium.org/7229013/diff/17005/media/video/capture/win/fil... > > media/video/capture/win/filter_base_win.cc:36: return 1; > > On 2011/06/28 08:57:23, tommi wrote: > > > On 2011/06/27 18:10:27, cpu wrote: > > > > it is a little bit puzzling returning 1, in here and elsewhere in this > code. > > > > While it is true that nobody normally should care about AddRef and Release > > > > return values, returning always 1 means you forfeit some avenues for > > > debugging. > > > > > > I absolutely agree. I asked Per to do it this way however because the > > reference > > > counting is already being done by base::RefCounted<> and I don't want to > start > > a > > > pattern of having two reference counters in COM classes just because the > > > RefCounted implementation returns void. Imho it would be useful to change > > > RefCounted's AddRef/Release to return the reference number but I'm sure > there > > > was a bike shed discussion on that issue back in the day. > > > > Seeing as we duplicate the impl of AddRef/Release in this + other classes, > would > > it help to define a helper base class that implements it w/ the return 1 and a > > comment describing why? > > > > Something like: > > > > // We need to do this because of blah blah blah > > template <class T> > > class DShowRefCounted : public base::RefCounted<T> { > > public: > > STDMETHOD_(ULONG, AddRef)() { > > base::RefCounted<T>::AddRef(); > > return 1; > > } > > > > STDMETHOD_(ULONG, Release)() { > > base::RefCounted<T>::Release(); > > return 1; > > } > > } > > > > I discussed this today with tommi. The tricky part is that PinBase inherits from > two different I-classes that both inherits from IUnknown. > tommi have a suggested solution but it got to complicated for this patch and we > therefore suggest a generic COM refcount classes is added among the other COM > base classes and this patch is later changed to use it. Shucks :( Chromium's AddRef/Release is compile-time and isn't part of an interface... I had a feeling the MS interfaces might interfere! > > http://codereview.chromium.org/7229013/diff/22002/media/media.gyp > > File media/media.gyp (right): > > > > http://codereview.chromium.org/7229013/diff/22002/media/media.gyp#newcode18 > > media/media.gyp:18: '../build/temp_gyp/googleurl.gyp:googleurl', > > ????? > > > > media should not depend on googleurl
On 2011/06/29 13:40:49, Per K wrote: > http://codereview.chromium.org/7229013/diff/22002/media/media.gyp > File media/media.gyp (right): > > http://codereview.chromium.org/7229013/diff/22002/media/media.gyp#newcode18 > media/media.gyp:18: '../build/temp_gyp/googleurl.gyp:googleurl', > ????? This is not part of this patch. It was added by alcolwell in rev 79185 it > seems like. Ah! Apologies -- I was looking at the inter-patch-diff and not the diff-from-trunk. I'll go check w/ acolwell.
but yes still LGTM from me -- tommi want to take it from here?
Yes I will take it from here. Thanks Andrew.
Regarding the AddRef/Release problem, it is not really specific to COM or MS
although that's where we're likely to run into it.
It's a problem with polymorphism and common pure virtual base classes (i.e.
interfaces).
If you inherit from multiple classes that all inherit from a common
interface,
then each of those base classes needs an implementation for each of those
pure
virtual methods. In our case AddRef/Release and QueryInterface.
For instance in this case:
struct I {
virtual void Foo() = 0; // our pure virtual method
}
struct A : public I {
virtual void BarA() {}
};
struct B : public I {
virtual void BarB() {}
};
struct MostDerived
: public A,
public B {
virtual void Foo() {
// Am I A::Foo() or B::Foo()?
}
};
The MostDerived::Foo method will actually satisfy both A::Foo and B::Foo.
However, if you move the implementation of Foo() to a third base class:
struct C {
virtual void Foo() {}
};
and do:
struct MostDerived : public A, public B, public C {
};
The compiler won't find implementations for A::Foo or B::Foo.
If we use a template instead, we get a step closer:
template <class T>
struct C : public T {
virtual void Foo() { /* implementation */}
};
and do:
struct MostDerived : public C<A>, public C<B> {
};
However, now we truly have two separate implementations for Foo().
This is the reason we cannot do this for AddRef/Release as we don't want
to have a reference count for each interface (it should be per object).
So, something like the below will work, but there's more to it when it comes
to
COM since we should also support a way for the 'MostDerived' class
to declare a list of supported interfaces (or we could make it a requirement
that the implementation for QueryInterface is always in the class itself).
Note that with this approach, we actually change what class is the most
derived class and instead of MostDerived, it will become our template class:
struct MostDerived : public A, B {
// no implementation for Foo() here.
};
template<class T>
struct IImplementation : public T {
virtual void Foo() { /* implementation */ }
};
// Here we actually provide the implementation of Foo().
IImplementation<MostDerived>* my_object =
new IImplementation<MostDerived>();
my_object->Foo();
So, after discussing this with Per, we agreed that this work is more inline
with some other
COM classes that we have such as ScopedComPtr, ScopedVariant, ScopedBstr and
the COM classes in Chrome Frame. Since I have done work on those in the
past I
volunteered to do this and I don't think it's worth our time dragging such a
change
into this changelist as it would also make sense to add a few new reviewers
and
COM specialists.
Long story short, it's simplest to keep the current implementation of
AddRef/Release
as it is :-) I hope that's alright by you.
Tommi
On Wed, Jun 29, 2011 at 7:06 PM, <scherkus@chromium.org> wrote:
> but yes still LGTM from me -- tommi want to take it from here?
>
>
>
http://codereview.chromium.**org/7229013/<http://codereview.chromium.org/7229...
>
On Wed, Jun 29, 2011 at 2:52 PM, Tommi <tommi@chromium.org> wrote: > Yes I will take it from here. Thanks Andrew. > > Regarding the AddRef/Release problem, it is not really specific to COM or > MS > although that's where we're likely to run into it. > It's a problem with polymorphism and common pure virtual base classes (i.e. > interfaces). > If you inherit from multiple classes that all inherit from a common > interface, > then each of those base classes needs an implementation for each of those > pure > virtual methods. In our case AddRef/Release and QueryInterface. > > For instance in this case: > > struct I { > virtual void Foo() = 0; // our pure virtual method > } > > struct A : public I { > virtual void BarA() {} > }; > > struct B : public I { > virtual void BarB() {} > }; > > struct MostDerived > : public A, > public B { > virtual void Foo() { > // Am I A::Foo() or B::Foo()? > } > }; > > The MostDerived::Foo method will actually satisfy both A::Foo and B::Foo. > However, if you move the implementation of Foo() to a third base class: > > struct C { > virtual void Foo() {} > }; > > and do: > > struct MostDerived : public A, public B, public C { > }; > > The compiler won't find implementations for A::Foo or B::Foo. > > If we use a template instead, we get a step closer: > > template <class T> > struct C : public T { > virtual void Foo() { /* implementation */} > }; > > and do: > > struct MostDerived : public C<A>, public C<B> { > }; > > However, now we truly have two separate implementations for Foo(). > This is the reason we cannot do this for AddRef/Release as we don't want > to have a reference count for each interface (it should be per object). > > So, something like the below will work, but there's more to it when it > comes to > COM since we should also support a way for the 'MostDerived' class > to declare a list of supported interfaces (or we could make it a > requirement > that the implementation for QueryInterface is always in the class itself). > Note that with this approach, we actually change what class is the most > derived class and instead of MostDerived, it will become our template > class: > > struct MostDerived : public A, B { > // no implementation for Foo() here. > }; > > template<class T> > struct IImplementation : public T { > virtual void Foo() { /* implementation */ } > }; > > // Here we actually provide the implementation of Foo(). > IImplementation<MostDerived>* my_object = > new IImplementation<MostDerived>(); > my_object->Foo(); > > So, after discussing this with Per, we agreed that this work is more inline > with some other > COM classes that we have such as ScopedComPtr, ScopedVariant, ScopedBstr > and > the COM classes in Chrome Frame. Since I have done work on those in the > past I > volunteered to do this and I don't think it's worth our time dragging such > a change > into this changelist as it would also make sense to add a few new reviewers > and > COM specialists. > > Long story short, it's simplest to keep the current implementation of > AddRef/Release > as it is :-) I hope that's alright by you. > > Tommi > Yeah it's OK -- it was an off-the-cuff suggestion :) Andrew On Wed, Jun 29, 2011 at 7:06 PM, <scherkus@chromium.org> wrote: > >> but yes still LGTM from me -- tommi want to take it from here? >> >> >> http://codereview.chromium.**org/7229013/<http://codereview.chromium.org/7229... >> > >
Change committed as 91096 |
