File Video Capture: allow IO operations from Device thread

File Video Capture: allow IO operations from Device thread

FileVideoCapture is a test-only feature, enabled via two
command line flags. It overwrites any system webcam and
introduces a file playback instead. This CL allows the
video capture thread to access IO operations.

BUG=648989
TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream --use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/

Committed: https://crrev.com/831eae63921bd78ee3d8da4d017bc439a3642985
Cr-Commit-Position: refs/heads/master@{#420503}

[mcasas, 2016-09-21 17:04:07]

Description was changed from

==========
File Video Capture: allow IO operations from Device thread

BUG=648989
==========

to

==========
File Video Capture: allow IO operations from Device thread

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/
==========

[mcasas, 2016-09-21 17:31:48]

Description was changed from

==========
File Video Capture: allow IO operations from Device thread

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/
==========

to

==========
File Video Capture: allow IO operations from Device thread

FileVideoCapture is a test-only feature, enabled via two
command line flags. It overwrites any system webcam and
introduces a file playback instead. This CL allows the
video capture thread to access IO operations.

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/
==========

[mcasas, 2016-09-21 22:29:00]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-09-21 22:35:58]

mcasas@chromium.org changed reviewers:
+ marpan@chromium.org

[mcasas, 2016-09-21 22:35:58]

marpan@ ptal

[mcasas, 2016-09-22 15:57:36]

mcasas@chromium.org changed reviewers:
+ chfremer@chromium.org
- marpan@chromium.org

[mcasas, 2016-09-22 15:57:36]

chfremer@ PTAL

[chfremer, 2016-09-22 16:39:54]

https://codereview.chromium.org/2358093002/diff/20001/media/capture/video/fil...
File media/capture/video/file_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2358093002/diff/20001/media/capture/video/fil...
media/capture/video/file_video_capture_device_factory.cc:45:
base::ThreadRestrictions::SetIOAllowed(true);
I believe this will work, but it feels a little hacky to simply allow IO
operations on the calling thread without telling the clients about it.

What would a clean solution look like?
I am guessing, we would we have to post the IO operations to the IO thread, wait
for the response and then return back to the client? Are there any arguments
against doing that?

[mcasas, 2016-09-22 16:47:03]

ptal

https://codereview.chromium.org/2358093002/diff/20001/media/capture/video/fil...
File media/capture/video/file_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2358093002/diff/20001/media/capture/video/fil...
media/capture/video/file_video_capture_device_factory.cc:45:
base::ThreadRestrictions::SetIOAllowed(true);
On 2016/09/22 16:39:53, chfremer wrote:
> I believe this will work, but it feels a little hacky to simply allow IO
> operations on the calling thread without telling the clients about it.

It is hacky, but the Video Capture thread is not ours to
deal with, it comes from the AudioManager [1], and we 
can't modify it there (would affect others).

> 
> What would a clean solution look like?
> I am guessing, we would we have to post the IO operations to the IO thread,
wait
> for the response and then return back to the client? Are there any arguments
> against doing that?

Yeah, the best-est way would be to post these tasks
to the FILE or the IO thread, I'd say, or allow the 
FileVCD to operate on IO thread.  The only argument
against this is complexity: for a Debug device, I 
think it's overkill to post to FILE/IO Thread, and 
arguably FileVCD should try to mimic any other VCD,
where there is at most 1 thread beyond the IO.

I could see FileVCD having an internal thread,
with IO allowed, to perform the Fie operations.
This would replicate the other VCD implementations
that have an internal thread, but IMHO this would
over complicate a debug-only feature.

[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

[chfremer, 2016-09-22 17:14:20]

On 2016/09/22 16:47:03, mcasas wrote:
> ptal
> 
>
https://codereview.chromium.org/2358093002/diff/20001/media/capture/video/fil...
> File media/capture/video/file_video_capture_device_factory.cc (right):
> 
>
https://codereview.chromium.org/2358093002/diff/20001/media/capture/video/fil...
> media/capture/video/file_video_capture_device_factory.cc:45:
> base::ThreadRestrictions::SetIOAllowed(true);
> On 2016/09/22 16:39:53, chfremer wrote:
> > I believe this will work, but it feels a little hacky to simply allow IO
> > operations on the calling thread without telling the clients about it.
> 
> It is hacky, but the Video Capture thread is not ours to
> deal with, it comes from the AudioManager [1], and we 
> can't modify it there (would affect others).
> 
> > 
> > What would a clean solution look like?
> > I am guessing, we would we have to post the IO operations to the IO thread,
> wait
> > for the response and then return back to the client? Are there any arguments
> > against doing that?
> 
> Yeah, the best-est way would be to post these tasks
> to the FILE or the IO thread, I'd say, or allow the 
> FileVCD to operate on IO thread.  The only argument
> against this is complexity: for a Debug device, I 
> think it's overkill to post to FILE/IO Thread, and 
> arguably FileVCD should try to mimic any other VCD,
> where there is at most 1 thread beyond the IO.
> 
> I could see FileVCD having an internal thread,
> with IO allowed, to perform the Fie operations.
> This would replicate the other VCD implementations
> that have an internal thread, but IMHO this would
> over complicate a debug-only feature.
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi...

Fair enough lgtm. Overall, I would prefer the added complexity 
over the hack. But I also feel that it seems like a lot of 
effort for this particular feature, so I'll let you make the call.

[mcasas, 2016-09-22 17:26:49]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-09-22 17:27:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 17:38:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 17:38:28]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mcasas, 2016-09-22 22:01:24]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-09-22 22:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
chfremer@chromium.org
Link to the patchset: https://codereview.chromium.org/2358093002/#ps40001
(title: "#include "base/threading/thread_restrictions.h"")

[commit-bot: I haz the power, 2016-09-22 22:02:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 23:45:23]

Description was changed from

==========
File Video Capture: allow IO operations from Device thread

FileVideoCapture is a test-only feature, enabled via two
command line flags. It overwrites any system webcam and
introduces a file playback instead. This CL allows the
video capture thread to access IO operations.

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/
==========

to

==========
File Video Capture: allow IO operations from Device thread

FileVideoCapture is a test-only feature, enabled via two
command line flags. It overwrites any system webcam and
introduces a file playback instead. This CL allows the
video capture thread to access IO operations.

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/
==========

[commit-bot: I haz the power, 2016-09-22 23:45:24]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-22 23:50:20]

Description was changed from

==========
File Video Capture: allow IO operations from Device thread

FileVideoCapture is a test-only feature, enabled via two
command line flags. It overwrites any system webcam and
introduces a file playback instead. This CL allows the
video capture thread to access IO operations.

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/
==========

to

==========
File Video Capture: allow IO operations from Device thread

FileVideoCapture is a test-only feature, enabled via two
command line flags. It overwrites any system webcam and
introduces a file playback instead. This CL allows the
video capture thread to access IO operations.

BUG=648989

TEST= Run e.g.
./out/gn/Chromium.app/Contents/MacOS/Chromium --use-fake-device-for-media-stream
--use-file-for-fake-video-capture=~/Downloads/old_town_cross_420_720p50.y4m
https://rawgit.com/Miguelao/demos/master/gum_simple.html

with the said file d'led from https://media.xiph.org/video/derf/y4m/

Committed: https://crrev.com/831eae63921bd78ee3d8da4d017bc439a3642985
Cr-Commit-Position: refs/heads/master@{#420503}
==========

[commit-bot: I haz the power, 2016-09-22 23:50:21]

Patchset 2 (id:??) landed as
https://crrev.com/831eae63921bd78ee3d8da4d017bc439a3642985
Cr-Commit-Position: refs/heads/master@{#420503}