From f18290df038fc2156ad5540a002aa4371c32b999 Mon Sep 17 00:00:00 2001 From: georgesak Date: Thu, 29 Oct 2015 12:52:45 -0700 Subject: Revert of webcamPrivate API: Fix regression from r351343. (patchset #7 id:130001 of https://codereview.chromium.org/1412673007/ ) Reason for revert: Suspected of breaking Chrome OS compilation. http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20GN%20%28dbg%29/builds/11367/steps/compile/logs/stdio Original issue's description: > webcamPrivate API: Fix regression from r351343. > > Additionally: > - Add unit test to test for this regression. > - Modify classes to support VISCA webcam unit tests. > - Modify VISCA webcam code to simplify code and to be more robust > against bad responses. > > BUG=548043 > > Committed: https://crrev.com/d088b80e0203d6f819a714d39ef87221a02549f9 > Cr-Commit-Position: refs/heads/master@{#356910} TBR=xdai@chromium.org,rockot@chromium.org,thestig@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=548043 Review URL: https://codereview.chromium.org/1428803003 Cr-Commit-Position: refs/heads/master@{#356920} --- .../browser/api/webcam_private/visca_webcam.cc | 183 +++++++++------------ .../browser/api/webcam_private/visca_webcam.h | 34 +--- .../api/webcam_private/visca_webcam_unittest.cc | 140 ---------------- .../webcam_private/webcam_private_api_chromeos.cc | 10 +- 4 files changed, 88 insertions(+), 279 deletions(-) delete mode 100644 extensions/browser/api/webcam_private/visca_webcam_unittest.cc (limited to 'extensions/browser/api/webcam_private') diff --git a/extensions/browser/api/webcam_private/visca_webcam.cc b/extensions/browser/api/webcam_private/visca_webcam.cc index 5eb4f02..d3cf70f 100644 --- a/extensions/browser/api/webcam_private/visca_webcam.cc +++ b/extensions/browser/api/webcam_private/visca_webcam.cc @@ -95,11 +95,6 @@ int ShiftResponseLowerBits(char c, size_t shift) { return static_cast(c & 0x0F) << shift; } -bool CanBuildResponseInt(const std::vector& response, - size_t start_index) { - return response.size() >= start_index + 4; -} - int BuildResponseInt(const std::vector& response, size_t start_index) { return ShiftResponseLowerBits(response[start_index], 12) + ShiftResponseLowerBits(response[start_index + 1], 8) + @@ -131,23 +126,25 @@ int GetPositiveValue(int value) { namespace extensions { -ViscaWebcam::ViscaWebcam() : pan_(0), tilt_(0), weak_ptr_factory_(this) {} +ViscaWebcam::ViscaWebcam(const std::string& path, + const std::string& extension_id) + : path_(path), + extension_id_(extension_id), + pan_(0), + tilt_(0), + weak_ptr_factory_(this) {} ViscaWebcam::~ViscaWebcam() { } -void ViscaWebcam::Open(const std::string& path, - const std::string& extension_id, - const OpenCompleteCallback& open_callback) { +void ViscaWebcam::Open(const OpenCompleteCallback& open_callback) { BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, base::Bind(&ViscaWebcam::OpenOnIOThread, weak_ptr_factory_.GetWeakPtr(), - path, extension_id, open_callback)); + open_callback)); } -void ViscaWebcam::OpenOnIOThread(const std::string& path, - const std::string& extension_id, - const OpenCompleteCallback& open_callback) { +void ViscaWebcam::OpenOnIOThread(const OpenCompleteCallback& open_callback) { DCHECK_CURRENTLY_ON(BrowserThread::IO); api::serial::ConnectionOptions options; @@ -164,7 +161,7 @@ void ViscaWebcam::OpenOnIOThread(const std::string& path, options.parity_bit = api::serial::PARITY_BIT_NO; options.stop_bits = api::serial::STOP_BITS_ONE; - serial_connection_.reset(new SerialConnection(path, extension_id)); + serial_connection_.reset(new SerialConnection(path_, extension_id_)); serial_connection_->Open( options, base::Bind(&ViscaWebcam::OnConnected, weak_ptr_factory_.GetWeakPtr(), open_callback)); @@ -224,7 +221,6 @@ void ViscaWebcam::Send(const std::vector& command, void ViscaWebcam::OnSendCompleted(const CommandCompleteCallback& callback, int bytes_sent, api::serial::SendError error) { - // TODO(xdai): Check |bytes_sent|? if (error == api::serial::SEND_ERROR_NONE) { ReceiveLoop(callback); } else { @@ -243,39 +239,34 @@ void ViscaWebcam::OnReceiveCompleted(const CommandCompleteCallback& callback, api::serial::ReceiveError error) { data_buffer_.insert(data_buffer_.end(), data.begin(), data.end()); - if (error != api::serial::RECEIVE_ERROR_NONE || data_buffer_.empty()) { + if (error == api::serial::RECEIVE_ERROR_NONE) { + // Loop until encounter the terminator. + if (static_cast(data_buffer_.back()) != kViscaTerminator) { + base::MessageLoop::current()->PostTask( + FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop, + weak_ptr_factory_.GetWeakPtr(), callback)); + } else { + // Clear |data_buffer_|. + std::vector response; + response.swap(data_buffer_); + + if ((static_cast(response[1]) & 0xF0) == kViscaResponseError) { + callback.Run(false, response); + } else if ((static_cast(response[1]) & 0xF0) != kViscaResponseAck && + (static_cast(response[1]) & 0xFF) != + kViscaResponseNetworkChange) { + callback.Run(true, response); + } else { + base::MessageLoop::current()->PostTask( + FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop, + weak_ptr_factory_.GetWeakPtr(), callback)); + } + } + } else { // Clear |data_buffer_|. std::vector response; response.swap(data_buffer_); callback.Run(false, response); - return; - } - - // Success case. If waiting for more data, then loop until encounter the - // terminator. - if (data_buffer_.back() != kViscaTerminator) { - base::MessageLoop::current()->PostTask( - FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop, - weak_ptr_factory_.GetWeakPtr(), callback)); - return; - } - - // Success case, and a complete response has been received. - // Clear |data_buffer_|. - std::vector response; - response.swap(data_buffer_); - - if (response.size() < 2 || - (static_cast(response[1]) & 0xF0) == kViscaResponseError) { - callback.Run(false, response); - } else if ((static_cast(response[1]) & 0xF0) != kViscaResponseAck && - (static_cast(response[1]) & 0xFF) != - kViscaResponseNetworkChange) { - callback.Run(true, response); - } else { - base::MessageLoop::current()->PostTask( - FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop, - weak_ptr_factory_.GetWeakPtr(), callback)); } } @@ -285,73 +276,58 @@ void ViscaWebcam::OnCommandCompleted(const SetPTZCompleteCallback& callback, // TODO(xdai): Error handling according to |response|. BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(callback, success)); - ProcessNextCommand(); + commands_.pop_front(); + + // If there are pending commands, process the next one. + if (!commands_.empty()) { + const std::vector next_command = commands_.front().first; + const CommandCompleteCallback next_callback = commands_.front().second; + serial_connection_->Send( + next_command, + base::Bind(&ViscaWebcam::OnSendCompleted, + weak_ptr_factory_.GetWeakPtr(), next_callback)); + } } void ViscaWebcam::OnInquiryCompleted(InquiryType type, const GetPTZCompleteCallback& callback, bool success, const std::vector& response) { + int value = 0; if (!success) { BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, - base::Bind(callback, false, 0)); - ProcessNextCommand(); - return; - } - - bool is_valid_response = false; - switch (type) { - case INQUIRY_PAN: - is_valid_response = CanBuildResponseInt(response, 2); - break; - case INQUIRY_TILT: - is_valid_response = CanBuildResponseInt(response, 6); - break; - case INQUIRY_ZOOM: - is_valid_response = CanBuildResponseInt(response, 2); - break; - } - if (!is_valid_response) { + base::Bind(callback, false, value)); + } else { + switch (type) { + case INQUIRY_PAN: + // See kGetPanTiltCommand for the format of response. + pan_ = BuildResponseInt(response, 2); + value = GetPositiveValue(pan_); + break; + case INQUIRY_TILT: + // See kGetPanTiltCommand for the format of response. + tilt_ = BuildResponseInt(response, 6); + value = GetPositiveValue(tilt_); + break; + case INQUIRY_ZOOM: + // See kGetZoomCommand for the format of response. + value = BuildResponseInt(response, 2); + break; + } BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, - base::Bind(callback, false, 0)); - ProcessNextCommand(); - return; - } - - int value = 0; - switch (type) { - case INQUIRY_PAN: - // See kGetPanTiltCommand for the format of response. - pan_ = BuildResponseInt(response, 2); - value = GetPositiveValue(pan_); - break; - case INQUIRY_TILT: - // See kGetPanTiltCommand for the format of response. - tilt_ = BuildResponseInt(response, 6); - value = GetPositiveValue(tilt_); - break; - case INQUIRY_ZOOM: - // See kGetZoomCommand for the format of response. - value = BuildResponseInt(response, 2); - break; + base::Bind(callback, true, value)); } - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, - base::Bind(callback, true, value)); - ProcessNextCommand(); -} - -void ViscaWebcam::ProcessNextCommand() { commands_.pop_front(); - if (commands_.empty()) - return; - // If there are pending commands, process the next one. - const std::vector next_command = commands_.front().first; - const CommandCompleteCallback next_callback = commands_.front().second; - serial_connection_->Send( - next_command, base::Bind(&ViscaWebcam::OnSendCompleted, - weak_ptr_factory_.GetWeakPtr(), next_callback)); + if (!commands_.empty()) { + const std::vector next_command = commands_.front().first; + const CommandCompleteCallback next_callback = commands_.front().second; + serial_connection_->Send( + next_command, + base::Bind(&ViscaWebcam::OnSendCompleted, + weak_ptr_factory_.GetWeakPtr(), next_callback)); + } } void ViscaWebcam::PostOpenFailureTask( @@ -411,7 +387,7 @@ void ViscaWebcam::SetTilt(int value, } void ViscaWebcam::SetZoom(int value, const SetPTZCompleteCallback& callback) { - int actual_value = std::max(value, 0); + int actual_value = std::min(value, 0); std::vector command = CHAR_VECTOR_FROM_ARRAY(kSetZoomCommand); ResponseToCommand(&command, 4, actual_value); Send(command, base::Bind(&ViscaWebcam::OnCommandCompleted, @@ -483,13 +459,4 @@ void ViscaWebcam::Reset(bool pan, } } -void ViscaWebcam::OpenForTesting( - scoped_ptr serial_connection) { - serial_connection_ = serial_connection.Pass(); -} - -SerialConnection* ViscaWebcam::GetSerialConnectionForTesting() { - return serial_connection_.get(); -} - } // namespace extensions diff --git a/extensions/browser/api/webcam_private/visca_webcam.h b/extensions/browser/api/webcam_private/visca_webcam.h index 7277788..7b56196 100644 --- a/extensions/browser/api/webcam_private/visca_webcam.h +++ b/extensions/browser/api/webcam_private/visca_webcam.h @@ -17,20 +17,18 @@ namespace extensions { class ViscaWebcam : public Webcam { public: - ViscaWebcam(); + ViscaWebcam(const std::string& path, const std::string& extension_id); using OpenCompleteCallback = base::Callback; // Open and initialize the web camera. This is done by the following three - // steps (in order): 1. Open the serial port; 2. Request address; 3. Clear the - // command buffer. After these three steps completes, |open_callback| will be - // called. - void Open(const std::string& path, - const std::string& extension_id, - const OpenCompleteCallback& open_callback); + // steps (in order): 1. Open the serial port; 2. Request address; 3. Clear + // the command buffer. After these three steps completes, |open_callback| will + // be called. + void Open(const OpenCompleteCallback& open_callback); private: - friend class ViscaWebcamTest; + ~ViscaWebcam() override; enum InquiryType { INQUIRY_PAN, @@ -41,23 +39,15 @@ class ViscaWebcam : public Webcam { using CommandCompleteCallback = base::Callback&)>; - // Private because WebCam is base::RefCounted. - ~ViscaWebcam() override; - - void OpenOnIOThread(const std::string& path, - const std::string& extension_id, - const OpenCompleteCallback& open_callback); - + void OpenOnIOThread(const OpenCompleteCallback& open_callback); // Callback function that will be called after the serial connection has been // opened successfully. void OnConnected(const OpenCompleteCallback& open_callback, bool success); - // Callback function that will be called after the address has been set // successfully. void OnAddressSetCompleted(const OpenCompleteCallback& open_callback, bool success, const std::vector& response); - // Callback function that will be called after the command buffer has been // cleared successfully. void OnClearAllCompleted(const OpenCompleteCallback& open_callback, @@ -87,7 +77,6 @@ class ViscaWebcam : public Webcam { bool success, const std::vector& response); - void ProcessNextCommand(); void PostOpenFailureTask(const OpenCompleteCallback& open_callback); // Webcam Overrides: @@ -112,18 +101,13 @@ class ViscaWebcam : public Webcam { bool zoom, const SetPTZCompleteCallback& callback) override; - // Used only in unit tests in place of Open(). - void OpenForTesting(scoped_ptr serial_connection); - - // Used only in unit tests to retrieve |serial_connection_| since this class - // owns it. - SerialConnection* GetSerialConnectionForTesting(); + const std::string path_; + const std::string extension_id_; scoped_ptr serial_connection_; // Stores the response for the current command. std::vector data_buffer_; - // Queues commands till the current command completes. std::deque, CommandCompleteCallback>> commands_; diff --git a/extensions/browser/api/webcam_private/visca_webcam_unittest.cc b/extensions/browser/api/webcam_private/visca_webcam_unittest.cc deleted file mode 100644 index fa97f0a..0000000 --- a/extensions/browser/api/webcam_private/visca_webcam_unittest.cc +++ /dev/null @@ -1,140 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include - -#include "content/public/test/test_browser_thread_bundle.h" -#include "extensions/browser/api/webcam_private/visca_webcam.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace extensions { - -namespace { - -class TestSerialConnection : public SerialConnection { - public: - TestSerialConnection() : SerialConnection("dummy_path", "dummy_id") {} - ~TestSerialConnection() override {} - - void SetReceiveBuffer(const std::vector& receive_buffer) { - receive_buffer_ = receive_buffer; - } - - void CheckSendBufferAndClear(const std::vector& expectations) { - EXPECT_EQ(send_buffer_, expectations); - send_buffer_.clear(); - } - - private: - // SerialConnection: - void Open(const api::serial::ConnectionOptions& options, - const OpenCompleteCallback& callback) override { - NOTREACHED(); - } - - bool Receive(const ReceiveCompleteCallback& callback) override { - callback.Run(receive_buffer_, api::serial::RECEIVE_ERROR_NONE); - receive_buffer_.clear(); - return true; - } - - bool Send(const std::vector& data, - const SendCompleteCallback& callback) override { - send_buffer_.insert(send_buffer_.end(), data.begin(), data.end()); - callback.Run(data.size(), api::serial::SEND_ERROR_NONE); - return true; - } - - std::vector receive_buffer_; - std::vector send_buffer_; - - DISALLOW_COPY_AND_ASSIGN(TestSerialConnection); -}; - -class GetPTZExpectations { - public: - GetPTZExpectations(bool expected_success, int expected_value) - : expected_success_(expected_success), expected_value_(expected_value) {} - - void OnCallback(bool success, int value) { - EXPECT_EQ(expected_success_, success); - EXPECT_EQ(expected_value_, value); - } - - private: - const bool expected_success_; - const int expected_value_; - - DISALLOW_COPY_AND_ASSIGN(GetPTZExpectations); -}; - -class SetPTZExpectations { - public: - explicit SetPTZExpectations(bool expected_success) - : expected_success_(expected_success) {} - - void OnCallback(bool success) { EXPECT_EQ(expected_success_, success); } - - private: - const bool expected_success_; - - DISALLOW_COPY_AND_ASSIGN(SetPTZExpectations); -}; - -#define CHAR_VECTOR_FROM_ARRAY(array) \ - std::vector(array, array + arraysize(array)) - -} // namespace - -class ViscaWebcamTest : public testing::Test { - protected: - ViscaWebcamTest() { - webcam_ = new ViscaWebcam; - webcam_->OpenForTesting(make_scoped_ptr(new TestSerialConnection)); - } - ~ViscaWebcamTest() override {} - - Webcam* webcam() { return webcam_.get(); } - - TestSerialConnection* serial_connection() { - return static_cast( - webcam_->GetSerialConnectionForTesting()); - } - - private: - content::TestBrowserThreadBundle thread_bundle_; - scoped_refptr webcam_; -}; - -TEST_F(ViscaWebcamTest, Zoom) { - // Check getting the zoom. - const char kGetZoomCommand[] = {0x81, 0x09, 0x04, 0x47, 0xFF}; - const char kGetZoomResponse[] = {0x00, 0x50, 0x01, 0x02, 0x03, 0x04, 0xFF}; - serial_connection()->SetReceiveBuffer( - CHAR_VECTOR_FROM_ARRAY(kGetZoomResponse)); - Webcam::GetPTZCompleteCallback receive_callback = - base::Bind(&GetPTZExpectations::OnCallback, - base::Owned(new GetPTZExpectations(true, 0x1234))); - webcam()->GetZoom(receive_callback); - serial_connection()->CheckSendBufferAndClear( - CHAR_VECTOR_FROM_ARRAY(kGetZoomCommand)); - - // Check setting the zoom. - const char kSetZoomCommand[] = {0x81, 0x01, 0x04, 0x47, 0x06, - 0x02, 0x05, 0x03, 0xFF}; - // Note: this is a valid, but empty value because nothing is checking it. - const char kSetZoomResponse[] = {0x00, 0x50, 0x00, 0x00, 0x00, 0x00, 0xFF}; - serial_connection()->SetReceiveBuffer( - CHAR_VECTOR_FROM_ARRAY(kSetZoomResponse)); - Webcam::SetPTZCompleteCallback send_callback = - base::Bind(&SetPTZExpectations::OnCallback, - base::Owned(new SetPTZExpectations(true))); - serial_connection()->SetReceiveBuffer( - CHAR_VECTOR_FROM_ARRAY(kSetZoomResponse)); - webcam()->SetZoom(0x6253, send_callback); - serial_connection()->CheckSendBufferAndClear( - CHAR_VECTOR_FROM_ARRAY(kSetZoomCommand)); -} - -} // namespace extensions diff --git a/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc b/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc index 02edcfa..ca7ad41 100644 --- a/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc +++ b/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc @@ -76,12 +76,10 @@ bool WebcamPrivateAPI::OpenSerialWebcam( if (webcam_resource) return false; - ViscaWebcam* visca_webcam = new ViscaWebcam; - visca_webcam->Open( - device_path, extension_id, - base::Bind(&WebcamPrivateAPI::OnOpenSerialWebcam, - weak_ptr_factory_.GetWeakPtr(), extension_id, device_path, - make_scoped_refptr(visca_webcam), callback)); + ViscaWebcam* visca_webcam(new ViscaWebcam(device_path, extension_id)); + visca_webcam->Open(base::Bind( + &WebcamPrivateAPI::OnOpenSerialWebcam, weak_ptr_factory_.GetWeakPtr(), + extension_id, device_path, make_scoped_refptr(visca_webcam), callback)); return true; } -- cgit v1.1