Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619503003:
Split FakeVideoCaptureDevice into classes with single responsibility (Closed)
|
Created:
3 years, 11 months ago by chfremer Modified:
3 years, 10 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit FakeVideoCaptureDevice into classes with single responsibility
This is mostly a refactoring in preparation of adding support for generating
MJPEG as output format as part of the Mojo Video Capture work [1].
For motiviation of separating responsibilities into classes, see [2].
There are two small fixes that change behavior:
1.) Before this CL, when switching the frame delivery to USE_CLIENT_BUFFERS, the
default pixel format (for all devices except the 2nd) was switched to ARGB.
FakeVideoCaptureDeviceFactory::GetSupportedFormats() would erroneously always
report I420 as pixel format, even if ARGB was delivered. Trying to actually use
USE_CLIENT_BUFFERS with ARGB anywhere beyond the unit tests, (e.g. using
command-line [4]) would crash with a failed CHECK, because ARGB is not actually
supported downstream.
After this CL, the default pixel format stays I420, even when switching to
USE_CLIENT_BUFFERS.
2.) Fixed race condition that could cause the frame delivery from an old session
to continue when device is restarted.
In order to make reviewing easier, the split out code has not yet been moved to
separate files and methods are ordered such that they match up with the
equivalent code before the change. This makes it easier to see from the diff
what parts of the code have not been touched.
Here is an overview of what has moved where:
* Creating an instance of FakeVideoCaptureDevice with the same behavior as
before now requires some logic for correctly setting up its dependencies. This
logic is located in class FakeVideoCaptureDeviceMaker (because the name
FakeVideoCaptureDeviceFactory was already taken).
* The file-private logic for painting Pacman frames has moved to class
PacmanFramePainter with abstraction interface FramePainter.
* The logic for delivering frames to the client has moved to a strategy object
[3] with interface FrameDeliveryStrategy. The two variants have moved to the
implementations called OwnBufferFrameDeliveryStrategy and
ClientBufferFrameDeliveryStrategy respectively.
* Fake photo functionality has moved to class FakePhotoDevice
* The previously duplicated constants for supported frame sizes have been
consolidated to kSupportedSizes[], and FakeVideoCaptureDeviceFactory now asks
FakeVideoCaptureDevice for its supported sizes via a static method.
Updated the unit tests to cover more of the configurable parameter space and
sweep through several resolutions.
BUG=584797
TEST=
out/Default/capture_unittests --gtest_filter="FakeVideoCaptureDevice*"
out/Default/chrome --use-fake-device-for-media-stream https://webrtc.github.io/samples/src/content/getusermedia/gum/
out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/
out/Default/chrome --use-fake-device-for-media-stream=device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/
out/Default/chrome --use-fake-device-for-media-stream=ownership=client,device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] https://en.wikipedia.org/wiki/Single_responsibility_principle
[3] https://en.wikipedia.org/wiki/Strategy_pattern
[4] out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/
Review-Url: https://codereview.chromium.org/2619503003
Cr-Commit-Position: refs/heads/master@{#450847}
Committed: https://chromium.googlesource.com/chromium/src/+/f2feab4a2a71b0c5fcbafbde747bde549029e0b7
Patch Set 1 #
Total comments: 21
Patch Set 2 : mcasas comments & rebase #
Total comments: 9
Patch Set 3 : emircan's comments #Patch Set 4 : rebase #
Total comments: 18
Patch Set 5 : mcasas@ suggestions #Patch Set 6 : mcasas@ suggestions pt2 #Patch Set 7 : Removed FramePainter interface #
Total comments: 9
Patch Set 8 : mcasas@ suggestions #
Messages
Total messages: 45 (28 generated)
Description was changed from ========== presubmit Fixed race condition that could cause the frame delivery from an old session to continue when device is restarted. Updated tests to sweep through several resolutions. Always use I420 as default format. Verify GetSupporteFormats() in unit test. Update tests. Restore behavior that using client buffers switches to ARGB Refactoring of FakeVideoCaptureDevice Salvage some diff with conflicts from 3 month old attempt. BUG= ========== to ========== Split FakeVideoCaptureDevice into classes with single responsibility This is mostly a refactoring in preparation of adding support for generating MJPEG as output format as part of the Mojo Video Capture work [1]. For motiviation of separating responsibilities into classes, see [2]. There are two small fixes that change behavior: 1.) Before this CL, when switching the frame delivery to USE_CLIENT_BUFFERS, the default pixel format (for all devices except the 2nd) was switched to ARGB. FakeVideoCaptureDeviceFactory::GetSupportedFormats() would erroneously always report I420 as pixel format, even if ARGB was delivered. Trying to actually use USE_CLIENT_BUFFERS with ARGB anywhere beyond the unit tests, (e.g. using command-line [4]) would crash with a failed CHECK, because ARGB is not actually supported downstream. After this CL, the default pixel format stays I420, even when switching to USE_CLIENT_BUFFERS. 2.) Fixed race condition that could cause the frame delivery from an old session to continue when device is restarted. In order to make reviewing easier, the split out code has not yet been moved to separate files and methods are ordered such that they match up with the equivalent code before the change. This makes it easier to see from the diff what parts of the code have not been touched. Here is an overview of what has moved where: * Creating an instance of FakeVideoCaptureDevice with the same behavior as before now requires some logic for correctly setting up its dependencies. This logic is located in class FakeVideoCaptureDeviceMaker (because the name FakeVideoCaptureDeviceFactory was already taken). * The file-private logic for painting Pacman frames has moved to class PacmanFramePainter with abstraction interface FramePainter. * The logic for delivering frames to the client has moved to a strategy object [3] with interface FrameDeliveryStrategy. The two variants have moved to the implementations called OwnBufferFrameDeliveryStrategy and ClientBufferFrameDeliveryStrategy respectively. * Fake photo functionality has moved to class FakePhotoDevice * The previously duplicated constants for supported frame sizes have been consolidated to kSupportedSizes[], and FakeVideoCaptureDeviceFactory now asks FakeVideoCaptureDevice for its supported sizes via a static method. Updated the unit tests to cover more of the configurable parameter space and sweep through several resolutions. BUG=584797 TEST= out/Default/capture_unittests --gtest_filter="FakeVideoCaptureDevice*" out/Default/chrome --use-fake-device-for-media-stream https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=ownership=client,device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/ [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://en.wikipedia.org/wiki/Single_responsibility_principle [3] https://en.wikipedia.org/wiki/Strategy_pattern [4] out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/ ==========
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org
mcasas@: PTAL emircan@: FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/06 20:33:52, chfremer wrote: > mcasas@: PTAL > emircan@: FYI ping
had a few comments to get you started https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:26: class CAPTURE_EXPORT FakeVideoCaptureDeviceMaker { s/FakeVideoCaptureDeviceMaker/FakeVideoCaptureDeviceBuilder/? Maybe too java-ish but perhaps convey intention better than Maker. To follow that Java pattern, the method would be call Build(): https://en.wikipedia.org/wiki/Builder_pattern https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:28: enum class DeliveryMode { USE_OWN_BUFFERS, USE_CLIENT_BUFFERS }; I see what you mean with DeliveryMode, but the entries are Ownerships. I'd stick to s/DeliveryMode/BufferOwnership/ https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:39: struct CAPTURE_EXPORT FakeDeviceState { Classes only used inside a compilation/linking unit don't need CAPTURE_EXPORT, so please remove them where unnecessary. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:136: base::TimeTicks first_ref_time_; |device_state_|, |client_| and |first_ref_time_| are both in ClientBufferFrameDeliveryStrategy and OwnBufferFrameDeliveryStrategy, shouldn't they be moved up to FrameDeliveryStrategy ? https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:140: class CAPTURE_EXPORT FakePhotoDevice { All these classes would be better as inner classes of FakeVCD, and, except FakeVideoCaptureDeviceBuilder, try to forward declare them and defer their definition to the .cc file. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:181: static gfx::Size SnapToSupportedSize(const gfx::Size& requested_size); This doesn't need to be static in the class, can be a static function in the .cc file. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:106: } nit: no {} for one-line bodies. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:49: float output_frame_rate_; I'd leave this as |frame_rate_|, since is the only frame rate that makes sense in a VCDevice...
PTAL https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:26: class CAPTURE_EXPORT FakeVideoCaptureDeviceMaker { On 2017/01/12 03:19:16, mcasas wrote: > s/FakeVideoCaptureDeviceMaker/FakeVideoCaptureDeviceBuilder/? > Maybe too java-ish but perhaps convey intention better than > Maker. To follow that Java pattern, the method would be call > Build(): > https://en.wikipedia.org/wiki/Builder_pattern Actually, I intentionally avoided calling it Builder to avoid confusion with the Builder pattern, which is not being used here. This class merely exposes a single static factory method. I guess we could drop the class wrapper and expose the naked method in the namespace directly. Would you prefer that? Or would you still prefer ...Builder? I would be okay with either. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:28: enum class DeliveryMode { USE_OWN_BUFFERS, USE_CLIENT_BUFFERS }; On 2017/01/12 03:19:16, mcasas wrote: > I see what you mean with DeliveryMode, but the entries > are Ownerships. I'd stick to s/DeliveryMode/BufferOwnership/ I chose DeliveryMode as a name to highlight the fact that the different options use different sub-APIs (of the VideoCaptureDevice::Client interface) to deliver the frames. Who has ownership of the buffers seems secondary to that. When I first read the code and saw BufferOwnership, I had no idea what it means, so I was hoping this rename would be an improvement. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:39: struct CAPTURE_EXPORT FakeDeviceState { On 2017/01/12 03:19:16, mcasas wrote: > Classes only used inside a compilation/linking unit don't > need CAPTURE_EXPORT, so please remove them where > unnecessary. Done. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:136: base::TimeTicks first_ref_time_; On 2017/01/12 03:19:16, mcasas wrote: > |device_state_|, |client_| and |first_ref_time_| are > both in ClientBufferFrameDeliveryStrategy and > OwnBufferFrameDeliveryStrategy, shouldn't they > be moved up to FrameDeliveryStrategy ? In general, I am trying to avoid inheritance of base classes other than pure interfaces, because inheritance of base classes causes coupling which leads to many downsides when scales get larger. I guess in this small case it does not really hurt that much (other than my idealism). Done. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:140: class CAPTURE_EXPORT FakePhotoDevice { On 2017/01/12 03:19:16, mcasas wrote: > All these classes would be better as inner classes of FakeVCD, I don't see what the advantage of nesting classes would be here. The whole point of this refactoring is to separate things into self-contained units. Wouldn't nesting everything into FakeVCD then couple everything back together? > and, except FakeVideoCaptureDeviceBuilder, try to > forward declare them and defer their definition to the > .cc file. Good point. I moved everything not needed in the header to the cc. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:181: static gfx::Size SnapToSupportedSize(const gfx::Size& requested_size); On 2017/01/12 03:19:16, mcasas wrote: > This doesn't need to be static in the class, can be > a static function in the .cc file. Done. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.cc:106: } On 2017/01/12 03:19:16, mcasas wrote: > nit: no {} for one-line bodies. Done. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device_factory.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device_factory.h:49: float output_frame_rate_; On 2017/01/12 03:19:16, mcasas wrote: > I'd leave this as |frame_rate_|, since is the only > frame rate that makes sense in a VCDevice... Done.
https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:233: for (int i = 0; i < kSupportedSizesCount; i++) { One liner, you don't need {}. https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:628: base::TimeDelta::FromMicroseconds(1e6 / device_state_->format.frame_rate); Nit for defensive code, you can clamp the frame rate using kMaxFramesPerSecond. https://cs.chromium.org/chromium/src/media/base/limits.h?rcl=c39590d5eb4e5fb4... https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:656: if (!device_running_) Instead of using a bool and id to stop running tasks after stop, you can also just call InvalidateWeakPtrs() in StopAndDeAllocate(). https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:661: uint8_t* buffer = frame_delivery_strategy_->PrepareBufferForNextFrame(); uint8_t* const buffer
PTAL addressed emircan@'s comments https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:233: for (int i = 0; i < kSupportedSizesCount; i++) { On 2017/01/31 18:47:39, emircan wrote: > One liner, you don't need {}. Done. https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:628: base::TimeDelta::FromMicroseconds(1e6 / device_state_->format.frame_rate); On 2017/01/31 18:47:39, emircan wrote: > Nit for defensive code, you can clamp the frame rate using kMaxFramesPerSecond. > https://cs.chromium.org/chromium/src/media/base/limits.h?rcl=c39590d5eb4e5fb4... Hmm, interesting ... but this may not be the best location for this. I feel a better place would be somewhere closer to where a requested frame_rate could come from. I believe there is already something where fps get parsed from the command line in FakeVideoCaptureDeviceFactory. https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:656: if (!device_running_) On 2017/01/31 18:47:39, emircan wrote: > Instead of using a bool and id to stop running tasks after stop, you can also > just call InvalidateWeakPtrs() in StopAndDeAllocate(). Done. https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:661: uint8_t* buffer = frame_delivery_strategy_->PrepareBufferForNextFrame(); On 2017/01/31 18:47:38, emircan wrote: > uint8_t* const buffer Done.
lgtm https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:628: base::TimeDelta::FromMicroseconds(1e6 / device_state_->format.frame_rate); On 2017/02/01 00:21:18, chfremer wrote: > On 2017/01/31 18:47:39, emircan wrote: > > Nit for defensive code, you can clamp the frame rate using > kMaxFramesPerSecond. > > > https://cs.chromium.org/chromium/src/media/base/limits.h?rcl=c39590d5eb4e5fb4... > > Hmm, interesting ... but this may not be the best location for this. > I feel a better place would be somewhere closer to where a requested frame_rate > could come from. I believe there is already something where fps get parsed from > the command line in FakeVideoCaptureDeviceFactory. I see, there is a kFakeCaptureMaxFrameRate used when parsing which is even lower.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Couple of questions, almost there. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:26: class CAPTURE_EXPORT FakeVideoCaptureDeviceMaker { On 2017/01/18 01:29:52, chfremer wrote: > On 2017/01/12 03:19:16, mcasas wrote: > > s/FakeVideoCaptureDeviceMaker/FakeVideoCaptureDeviceBuilder/? > > Maybe too java-ish but perhaps convey intention better than > > Maker. To follow that Java pattern, the method would be call > > Build(): > > https://en.wikipedia.org/wiki/Builder_pattern > > Actually, I intentionally avoided calling it Builder to avoid confusion with the > Builder pattern, which is not being used here. This class merely exposes a > single static factory method. I guess we could drop the class wrapper and expose > the naked method in the namespace directly. Would you prefer that? Or would you > still prefer ...Builder? I would be okay with either. Well, if it's a Factory-like thing it should be called Factory, but since there is a Factory, isn't that a sign of code being repeated? Maybe some code could be moved to the Factory. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:28: enum class DeliveryMode { USE_OWN_BUFFERS, USE_CLIENT_BUFFERS }; On 2017/01/18 01:29:52, chfremer wrote: > On 2017/01/12 03:19:16, mcasas wrote: > > I see what you mean with DeliveryMode, but the entries > > are Ownerships. I'd stick to s/DeliveryMode/BufferOwnership/ > > I chose DeliveryMode as a name to highlight the fact that the different options > use different sub-APIs (of the VideoCaptureDevice::Client interface) to deliver > the frames. Who has ownership of the buffers seems secondary to that. When I > first read the code and saw BufferOwnership, I had no idea what it means, so I > was hoping this rename would be an improvement. Hmm makes sense, use the API naming and not the internal detail. In that case, s/OWN/DEVICE/? or s/OWN/INTERNAL/ ? https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:58: } No {} in one-line bodies. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:59: } for (const gfx::Size& supported_size : kSupportedSizes) { if (requested_size.width() <= supported_size.width()) return supported_size; } return arraysize(kSupportedSizes) - 1; (Not super sure if the last line will work though). Also, note that this algorithm implicitly relies on |kSupportedSizes| being ordered increasingly, from small to large sizes. Please add a comment at such effect. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:63: class FakeVideoCaptureDevice; Probably not needed? I don't see any refs to it before its definition in l.185 https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:82: }; Why a base class if there's only one class deriving from it? https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:547: if (client_ == nullptr) if (!client_) here and in l.556 https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:124: EXPECT_TRUE(storage == media::PIXEL_STORAGE_CPU); EXPECT_EQ(media::PIXEL_STORAGE_CPU, storage); https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:516: class FakeVideoCaptureDeviceFactoryTest Maybe we should split this into a file on its own fake_video_capture_device_factory_unittest.cc https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:582: PIXEL_FORMAT_I420})}, Here we could just use {PIXEL_FORMAT_I420, PIXEL_FORMAT_Y16, PIXEL_FORMAT_I420} and that's use the vector-from-array constructor, right? It'd be easier to read. (BTW cool to make this explicit).
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mcasas@: Please take another look. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:26: class CAPTURE_EXPORT FakeVideoCaptureDeviceMaker { On 2017/02/15 00:44:19, mcasas wrote: > On 2017/01/18 01:29:52, chfremer wrote: > > On 2017/01/12 03:19:16, mcasas wrote: > > > s/FakeVideoCaptureDeviceMaker/FakeVideoCaptureDeviceBuilder/? > > > Maybe too java-ish but perhaps convey intention better than > > > Maker. To follow that Java pattern, the method would be call > > > Build(): > > > https://en.wikipedia.org/wiki/Builder_pattern > > > > Actually, I intentionally avoided calling it Builder to avoid confusion with > the > > Builder pattern, which is not being used here. This class merely exposes a > > single static factory method. I guess we could drop the class wrapper and > expose > > the naked method in the namespace directly. Would you prefer that? Or would > you > > still prefer ...Builder? I would be okay with either. > > Well, if it's a Factory-like thing it should be called > Factory, but since there is a Factory, isn't that a > sign of code being repeated? Maybe some code could be > moved to the Factory. I would say it is a sign of delegation happening, but not a sign of duplication. The thing currently called FakeVideoCaptureDeviceFactory delegates to this new class for part of the process of fulfilling the CreateDevice() method contract. But each class is responsible for a different part of that process. FakeVideoCaptureDeviceFactory is responsible for knowing which DeliveryMode to use and determining which VideoPixelFormat to choose. FakeVideoCaptureDeviceMaker is responsible for creating instances of FakeVideoCaptureDevice using this information. To do this, it has to instantiate and wire up lots of private parts that FakeVideoCaptureDeviceFactory should not need to know about. We can only name one of them FakeVideoCaptureDeviceFactory, and for simplicity I would opt for letting that name stay with the class that currently already has it. The name FakeVideoCaptureDeviceMaker is obviously not great. As long as we can keep the two separate, I am open to any alternative naming suggestions. https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:28: enum class DeliveryMode { USE_OWN_BUFFERS, USE_CLIENT_BUFFERS }; On 2017/02/15 00:44:19, mcasas wrote: > On 2017/01/18 01:29:52, chfremer wrote: > > On 2017/01/12 03:19:16, mcasas wrote: > > > I see what you mean with DeliveryMode, but the entries > > > are Ownerships. I'd stick to s/DeliveryMode/BufferOwnership/ > > > > I chose DeliveryMode as a name to highlight the fact that the different > options > > use different sub-APIs (of the VideoCaptureDevice::Client interface) to > deliver > > the frames. Who has ownership of the buffers seems secondary to that. When I > > first read the code and saw BufferOwnership, I had no idea what it means, so I > > was hoping this rename would be an improvement. > > Hmm makes sense, use the API naming and not the > internal detail. In that case, s/OWN/DEVICE/? > or s/OWN/INTERNAL/ ? Excellent suggestion. For maximum clarity I'd like to include both words, and call it USE_DEVICE_INTERNAL_BUFFERS and USE_CLIENT_PROVIDED_BUFFERS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, I missed a couple of comment in my previous reply. Addressed in PatchSet 6 https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:58: } On 2017/02/15 00:44:19, mcasas wrote: > No {} in one-line bodies. Done. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:59: } On 2017/02/15 00:44:19, mcasas wrote: > for (const gfx::Size& supported_size : kSupportedSizes) { > if (requested_size.width() <= supported_size.width()) > return supported_size; > > } > return arraysize(kSupportedSizes) - 1; > > (Not super sure if the last line will work though). > > Also, note that this algorithm implicitly relies on > |kSupportedSizes| being ordered increasingly, from > small to large sizes. Please add a comment at such > effect. Done. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:63: class FakeVideoCaptureDevice; On 2017/02/15 00:44:19, mcasas wrote: > Probably not needed? I don't see any refs to it > before its definition in l.185 Done. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:82: }; On 2017/02/15 00:44:19, mcasas wrote: > Why a base class if there's only one class > deriving from it? Short answer: For abstraction and loose coupling. Slightly more elaborate answer: Users of FramePainter (i.e. classes FakePhotoDevice and FakeVideoCaptureDevice) do not need to know what implementation is being used. They should work the same with any FramePainter (including Mocks). Depending on the abstraction instead of a concrete implementation makes them testable and reusable without PacmanFramePainter (or any of its dependencies). Also, human readers do not have to read PacmanFramePainter (or any of its dependencies) in order to understand what FakePhotoDevice and FakeVideoCaptureDevice do. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:547: if (client_ == nullptr) On 2017/02/15 00:44:19, mcasas wrote: > if (!client_) > > here and in l.556 Done. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:124: EXPECT_TRUE(storage == media::PIXEL_STORAGE_CPU); On 2017/02/15 00:44:19, mcasas wrote: > EXPECT_EQ(media::PIXEL_STORAGE_CPU, storage); Done. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:516: class FakeVideoCaptureDeviceFactoryTest On 2017/02/15 00:44:19, mcasas wrote: > Maybe we should split this into a file on its own > fake_video_capture_device_factory_unittest.cc Agreed that we should split. Please allow me to defer this to after finishing the refactoring, because I have stashed changes to this file coming up in the follow-up CL. I added this to the list of cleanups in the Design Doc. https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:582: PIXEL_FORMAT_I420})}, On 2017/02/15 00:44:19, mcasas wrote: > Here we could just use > {PIXEL_FORMAT_I420, PIXEL_FORMAT_Y16, PIXEL_FORMAT_I420} > and that's use the vector-from-array constructor, right? > It'd be easier to read. > (BTW cool to make this explicit). Nice! Thanks.
https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... File media/capture/video/fake_video_capture_device.h (right): https://codereview.chromium.org/2619503003/diff/1/media/capture/video/fake_vi... media/capture/video/fake_video_capture_device.h:28: enum class DeliveryMode { USE_OWN_BUFFERS, USE_CLIENT_BUFFERS }; On 2017/02/15 01:30:11, chfremer wrote: > On 2017/02/15 00:44:19, mcasas wrote: > > On 2017/01/18 01:29:52, chfremer wrote: > > > On 2017/01/12 03:19:16, mcasas wrote: > > > > I see what you mean with DeliveryMode, but the entries > > > > are Ownerships. I'd stick to s/DeliveryMode/BufferOwnership/ > > > > > > I chose DeliveryMode as a name to highlight the fact that the different > > options > > > use different sub-APIs (of the VideoCaptureDevice::Client interface) to > > deliver > > > the frames. Who has ownership of the buffers seems secondary to that. When I > > > first read the code and saw BufferOwnership, I had no idea what it means, so > I > > > was hoping this rename would be an improvement. > > > > Hmm makes sense, use the API naming and not the > > internal detail. In that case, s/OWN/DEVICE/? > > or s/OWN/INTERNAL/ ? > > Excellent suggestion. For maximum clarity I'd like to include both words, and > call it USE_DEVICE_INTERNAL_BUFFERS and USE_CLIENT_PROVIDED_BUFFERS. ok!! https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:82: }; On 2017/02/15 18:11:29, chfremer wrote: > On 2017/02/15 00:44:19, mcasas wrote: > > Why a base class if there's only one class > > deriving from it? > > Short answer: For abstraction and loose coupling. > > Slightly more elaborate answer: Users of FramePainter (i.e. classes > FakePhotoDevice and FakeVideoCaptureDevice) do not need to know what > implementation is being used. They should work the same with any FramePainter > (including Mocks). Depending on the abstraction instead of a concrete > implementation makes them testable and reusable without PacmanFramePainter (or > any of its dependencies). Also, human readers do not have to read > PacmanFramePainter (or any of its dependencies) in order to understand what > FakePhotoDevice and FakeVideoCaptureDevice do. That's all good, but in Chromium we only abstract things when we need to. IIUC PacmanFramePainter is only used in this file, and it's the only implementation of FramePainter, so there's no need to hide behind abstractions. If you would have 2 implementations of FramePainter, I'd go with your rationale, but otherwise I think it's more complication than worth. Please remove it.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Removed FramePainter abstraction. PTAL https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/60001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:82: }; On 2017/02/15 18:23:47, mcasas wrote: > On 2017/02/15 18:11:29, chfremer wrote: > > On 2017/02/15 00:44:19, mcasas wrote: > > > Why a base class if there's only one class > > > deriving from it? > > > > Short answer: For abstraction and loose coupling. > > > > Slightly more elaborate answer: Users of FramePainter (i.e. classes > > FakePhotoDevice and FakeVideoCaptureDevice) do not need to know what > > implementation is being used. They should work the same with any FramePainter > > (including Mocks). Depending on the abstraction instead of a concrete > > implementation makes them testable and reusable without PacmanFramePainter (or > > any of its dependencies). Also, human readers do not have to read > > PacmanFramePainter (or any of its dependencies) in order to understand what > > FakePhotoDevice and FakeVideoCaptureDevice do. > > That's all good, but in Chromium we only abstract things > when we need to. IIUC PacmanFramePainter is only > used in this file, and it's the only implementation of > FramePainter, so there's no need to hide behind > abstractions. If you would have 2 implementations of > FramePainter, I'd go with your rationale, but otherwise > I think it's more complication than worth. Please remove > it. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with some suggestions https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:11: #include "base/atomicops.h" Probably not needed anymore :-) https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:81: // PIXEL_FORMAT_ARGB nit: Documentation is hard to maintain. Instead, DCHECK these values in the constructor? https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:98: class FrameDeliveryStrategy { I'm a bit confused as to this name, I'd say this is a FrameDeliverer, since what it does is to deliver frames (as opposed to what it is, i.e. a Strategy, which would be an architectural pattern, or something more of a policy class to be used in a template ?). And also, this is a BufferDeliverer, since it doesn't know what the contents of the buffer are precisely (ClientBufferFrameDeliveryStrategy::DeliverFrame() even calls a certain OnIncomingCapturedBuffer() ). But it seems that it knows the semantics of the |buffer_| it holds, so perhaps it should just own the PacmanFramePainter itself and call it in one single method the current lines l.645-647, e.g. void FrameDeliveryStrategy::DeliverNextFrame(base::TimeDelta timestamp) { frame_painter_->PaintFrame(timstamp, PrepareBufferForNextFrame()); DeliverFrame(); } With those two methods being protected and implemented in their respective classes. wdyt? https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:589: DCHECK_EQ(device_state_->format.pixel_storage, PIXEL_STORAGE_CPU); Expected goes first (as opposed to the JS test framework, where the expected goes second, confusingly enough).
Incorporated improvements suggested by mcasas@. Also, moved FrameDeliverer::first_ref_time_ from protected to private and removed some duplication in its use. https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:11: #include "base/atomicops.h" On 2017/02/15 19:06:52, mcasas wrote: > Probably not needed anymore :-) Done. https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:81: // PIXEL_FORMAT_ARGB On 2017/02/15 19:06:52, mcasas wrote: > nit: Documentation is hard to maintain. Instead, DCHECK these > values in the constructor? I am adding the DCHECKs in order to fail early. I'd like to keep the documentation here as well, because I'd like readers to be able to use the class without having to read the implementation. https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:98: class FrameDeliveryStrategy { On 2017/02/15 19:06:52, mcasas wrote: > I'm a bit confused as to this name, I'd say this is a > FrameDeliverer, since what it does is to deliver > frames (as opposed to what it is, i.e. a Strategy, > which would be an architectural pattern, or something > more of a policy class to be used in a template ?). > > And also, this is a BufferDeliverer, since it doesn't > know what the contents of the buffer are precisely > (ClientBufferFrameDeliveryStrategy::DeliverFrame() > even calls a certain OnIncomingCapturedBuffer() ). > > But it seems that it knows the semantics of the > |buffer_| it holds, so perhaps it should just own the > PacmanFramePainter itself and call it in one single > method the current lines l.645-647, e.g. > > void FrameDeliveryStrategy::DeliverNextFrame(base::TimeDelta timestamp) { > frame_painter_->PaintFrame(timstamp, PrepareBufferForNextFrame()); > DeliverFrame(); > } > > With those two methods being protected and implemented > in their respective classes. wdyt? > Thanks, I tried your suggestion and moved |frame_painter_| into FrameDeliverer. I like how it simplifies things, and eliminates the need for the ugly 2-step operation of PrepareBufferForNextFrame() and DeliverFrame() passing around raw pointer. As an implication of this change, FrameDeliverer now not only delivers but also paints/generates the frames. I don't think we need to rename the classes, but I changed the name of the method to PaintAndDeliverNextFrame(). wdyt? https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:589: DCHECK_EQ(device_state_->format.pixel_storage, PIXEL_STORAGE_CPU); On 2017/02/15 19:06:52, mcasas wrote: > Expected goes first (as opposed to the JS test framework, where > the expected goes second, confusingly enough). Done.
https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2619503003/diff/120001/media/capture/video/fa... media/capture/video/fake_video_capture_device.cc:98: class FrameDeliveryStrategy { On 2017/02/15 20:52:05, chfremer wrote: > On 2017/02/15 19:06:52, mcasas wrote: > > I'm a bit confused as to this name, I'd say this is a > > FrameDeliverer, since what it does is to deliver > > frames (as opposed to what it is, i.e. a Strategy, > > which would be an architectural pattern, or something > > more of a policy class to be used in a template ?). > > > > And also, this is a BufferDeliverer, since it doesn't > > know what the contents of the buffer are precisely > > (ClientBufferFrameDeliveryStrategy::DeliverFrame() > > even calls a certain OnIncomingCapturedBuffer() ). > > > > But it seems that it knows the semantics of the > > |buffer_| it holds, so perhaps it should just own the > > PacmanFramePainter itself and call it in one single > > method the current lines l.645-647, e.g. > > > > void FrameDeliveryStrategy::DeliverNextFrame(base::TimeDelta timestamp) { > > frame_painter_->PaintFrame(timstamp, PrepareBufferForNextFrame()); > > DeliverFrame(); > > } > > > > With those two methods being protected and implemented > > in their respective classes. wdyt? > > > > Thanks, I tried your suggestion and moved |frame_painter_| into FrameDeliverer. > I like how it simplifies things, and eliminates the need for the ugly 2-step > operation of PrepareBufferForNextFrame() and DeliverFrame() passing around raw > pointer. > > As an implication of this change, FrameDeliverer now not only delivers but also > paints/generates the frames. I don't think we need to rename the classes, but I > changed the name of the method to PaintAndDeliverNextFrame(). > > wdyt? I like it better! PS8 lgtm, thanks!
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2619503003/#ps140001 (title: "mcasas@ suggestions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487201803232270,
"parent_rev": "81ee0eb7c58ddf35cce08886ab5a6d4fc4303b71", "commit_rev":
"f2feab4a2a71b0c5fcbafbde747bde549029e0b7"}
Message was sent while issue was closed.
Description was changed from ========== Split FakeVideoCaptureDevice into classes with single responsibility This is mostly a refactoring in preparation of adding support for generating MJPEG as output format as part of the Mojo Video Capture work [1]. For motiviation of separating responsibilities into classes, see [2]. There are two small fixes that change behavior: 1.) Before this CL, when switching the frame delivery to USE_CLIENT_BUFFERS, the default pixel format (for all devices except the 2nd) was switched to ARGB. FakeVideoCaptureDeviceFactory::GetSupportedFormats() would erroneously always report I420 as pixel format, even if ARGB was delivered. Trying to actually use USE_CLIENT_BUFFERS with ARGB anywhere beyond the unit tests, (e.g. using command-line [4]) would crash with a failed CHECK, because ARGB is not actually supported downstream. After this CL, the default pixel format stays I420, even when switching to USE_CLIENT_BUFFERS. 2.) Fixed race condition that could cause the frame delivery from an old session to continue when device is restarted. In order to make reviewing easier, the split out code has not yet been moved to separate files and methods are ordered such that they match up with the equivalent code before the change. This makes it easier to see from the diff what parts of the code have not been touched. Here is an overview of what has moved where: * Creating an instance of FakeVideoCaptureDevice with the same behavior as before now requires some logic for correctly setting up its dependencies. This logic is located in class FakeVideoCaptureDeviceMaker (because the name FakeVideoCaptureDeviceFactory was already taken). * The file-private logic for painting Pacman frames has moved to class PacmanFramePainter with abstraction interface FramePainter. * The logic for delivering frames to the client has moved to a strategy object [3] with interface FrameDeliveryStrategy. The two variants have moved to the implementations called OwnBufferFrameDeliveryStrategy and ClientBufferFrameDeliveryStrategy respectively. * Fake photo functionality has moved to class FakePhotoDevice * The previously duplicated constants for supported frame sizes have been consolidated to kSupportedSizes[], and FakeVideoCaptureDeviceFactory now asks FakeVideoCaptureDevice for its supported sizes via a static method. Updated the unit tests to cover more of the configurable parameter space and sweep through several resolutions. BUG=584797 TEST= out/Default/capture_unittests --gtest_filter="FakeVideoCaptureDevice*" out/Default/chrome --use-fake-device-for-media-stream https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=ownership=client,device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/ [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://en.wikipedia.org/wiki/Single_responsibility_principle [3] https://en.wikipedia.org/wiki/Strategy_pattern [4] out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/ ========== to ========== Split FakeVideoCaptureDevice into classes with single responsibility This is mostly a refactoring in preparation of adding support for generating MJPEG as output format as part of the Mojo Video Capture work [1]. For motiviation of separating responsibilities into classes, see [2]. There are two small fixes that change behavior: 1.) Before this CL, when switching the frame delivery to USE_CLIENT_BUFFERS, the default pixel format (for all devices except the 2nd) was switched to ARGB. FakeVideoCaptureDeviceFactory::GetSupportedFormats() would erroneously always report I420 as pixel format, even if ARGB was delivered. Trying to actually use USE_CLIENT_BUFFERS with ARGB anywhere beyond the unit tests, (e.g. using command-line [4]) would crash with a failed CHECK, because ARGB is not actually supported downstream. After this CL, the default pixel format stays I420, even when switching to USE_CLIENT_BUFFERS. 2.) Fixed race condition that could cause the frame delivery from an old session to continue when device is restarted. In order to make reviewing easier, the split out code has not yet been moved to separate files and methods are ordered such that they match up with the equivalent code before the change. This makes it easier to see from the diff what parts of the code have not been touched. Here is an overview of what has moved where: * Creating an instance of FakeVideoCaptureDevice with the same behavior as before now requires some logic for correctly setting up its dependencies. This logic is located in class FakeVideoCaptureDeviceMaker (because the name FakeVideoCaptureDeviceFactory was already taken). * The file-private logic for painting Pacman frames has moved to class PacmanFramePainter with abstraction interface FramePainter. * The logic for delivering frames to the client has moved to a strategy object [3] with interface FrameDeliveryStrategy. The two variants have moved to the implementations called OwnBufferFrameDeliveryStrategy and ClientBufferFrameDeliveryStrategy respectively. * Fake photo functionality has moved to class FakePhotoDevice * The previously duplicated constants for supported frame sizes have been consolidated to kSupportedSizes[], and FakeVideoCaptureDeviceFactory now asks FakeVideoCaptureDevice for its supported sizes via a static method. Updated the unit tests to cover more of the configurable parameter space and sweep through several resolutions. BUG=584797 TEST= out/Default/capture_unittests --gtest_filter="FakeVideoCaptureDevice*" out/Default/chrome --use-fake-device-for-media-stream https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/ out/Default/chrome --use-fake-device-for-media-stream=ownership=client,device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/ [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://en.wikipedia.org/wiki/Single_responsibility_principle [3] https://en.wikipedia.org/wiki/Strategy_pattern [4] out/Default/chrome --use-fake-device-for-media-stream=ownership=client https://webrtc.github.io/samples/src/content/getusermedia/gum/ Review-Url: https://codereview.chromium.org/2619503003 Cr-Commit-Position: refs/heads/master@{#450847} Committed: https://chromium.googlesource.com/chromium/src/+/f2feab4a2a71b0c5fcbafbde747b... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/f2feab4a2a71b0c5fcbafbde747b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
