Update StreamTextureProxy to accept a base::Closure

Update StreamTextureProxy to accept a base::Closure

StreamTextureProxy (STP) is currently tightly coupled to the
VideoFrameProvider::Client (VFP::C). This forces classes that use STP to
be aware of the VFP::C, and forces them to include files from cc/layers.

This change updates STP to accept a custom callback to be triggered when
the STP's OnFrameAvailable() is signaled. It also moves
StreamTextureWrapper from media/blink to media/base/android, now that it
no longer needs to include cc/layers/video_frame_provider.h.

TEST= Made sure we could still use WMPA (opened an HLS stream on an
android device)
BUG=631178
Committed: https://crrev.com/f4d502c5ec011e3b5b3aea1669b1d6149d5b9aed
Cr-Commit-Position: refs/heads/master@{#409100}

[tguilbert, 2016-07-28 21:42:40]

tguilbert@chromium.org changed reviewers:
+ watk@chromium.org

[tguilbert, 2016-07-28 21:42:41]

PTAL :)

Thank you!
Thomas

[watk, 2016-07-29 19:03:16]

Nice one. I have about 50% confidence this is correct :) But that's about as
much as I'm going to get.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.cc:24: // Cannot call into
|client_| anymore (from any thread) after returning
|client_| is dead

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.h (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:40: // a connected
client will receive callbacks on. This can be called
update docs

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:42: void
BindToLoop(int32_t stream_id,
tangential: we should rename this at some point to BindToTaskRunner

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_wrapper_impl.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_wrapper_impl.cc:170: // Note:
StreamTextureProxy will release its reference to |client|
technically true, but it's more a callback now

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1254: if
(stream_texture_proxy_) {
For consistency we don't use braces for single line statements inside ifs

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1260:
video_frame_provider_client_ = client;
I'm not sure how this was ever thread safe, if this can be called on either the
compositor thread or render thread and it writes to a variable without a lock..
Not sure if a bug or not.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1314: base::Closure
frame_received_cb = base::Closure();
You can remove the "= base::Closure();"

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1322: //  - The Closure
is protected by a lock in StreamTextureProxy.
I would say the proof can be stated slightly more directly:
- |client| is valid until we receive a call to
SetVideoFrameProviderClient(nullptr)
- SetVideoFrameProviderClient(nullptr) clears proxy's callback guaranteeing it
will no longer be run.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1328:
stream_texture_proxy_->BindToLoop(stream_id_, frame_received_cb,
Same question as above. Is it thread safe to be accessing stream_texture_proxy_
and stream_id_ without locking?

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1350: if
(video_frame_provider_client_) {
no braces

[tguilbert, 2016-08-01 22:05:57]

TY for the comments. PTAL! 
(We can discuss thread safety offline if need be).

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.cc:24: // Cannot call into
|client_| anymore (from any thread) after returning
On 2016/07/29 19:03:16, watk wrote:
> |client_| is dead

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.h (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:40: // a connected
client will receive callbacks on. This can be called
On 2016/07/29 19:03:16, watk wrote:
> update docs

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:42: void
BindToLoop(int32_t stream_id,
On 2016/07/29 19:03:16, watk wrote:
> tangential: we should rename this at some point to BindToTaskRunner

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_wrapper_impl.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_wrapper_impl.cc:170: // Note:
StreamTextureProxy will release its reference to |client|
On 2016/07/29 19:03:16, watk wrote:
> technically true, but it's more a callback now

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1254: if
(stream_texture_proxy_) {
On 2016/07/29 19:03:16, watk wrote:
> For consistency we don't use braces for single line statements inside ifs

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1260:
video_frame_provider_client_ = client;
On 2016/07/29 19:03:16, watk wrote:
> I'm not sure how this was ever thread safe, if this can be called on either
the
> compositor thread or render thread and it writes to a variable without a
lock..
> Not sure if a bug or not.

My guess is that it is being used in a safe way, without being thread safe in of
itself. I don't think this method gets called more than once during
initialization and once or twice during teardown.

Because WMPA is on its way out and because this CL doesn't make things worse, I
am inclined to not add extra locks and checks.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1314: base::Closure
frame_received_cb = base::Closure();
On 2016/07/29 19:03:16, watk wrote:
> You can remove the "= base::Closure();" 

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1322: //  - The Closure
is protected by a lock in StreamTextureProxy.
On 2016/07/29 19:03:16, watk wrote:
> I would say the proof can be stated slightly more directly:
> - |client| is valid until we receive a call to
> SetVideoFrameProviderClient(nullptr)
> - SetVideoFrameProviderClient(nullptr) clears proxy's callback guaranteeing it
> will no longer be run.

Nicely said. Thank you!

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1328:
stream_texture_proxy_->BindToLoop(stream_id_, frame_received_cb,
On 2016/07/29 19:03:16, watk wrote:
> Same question as above. Is it thread safe to be accessing
stream_texture_proxy_
> and stream_id_ without locking?

I have not gone through the mental exercise of proving the thread safety/lack
thereof. I think that while it is theoretically not thread safe, in practice it
is not causing any problems.

As I mentioned above, I am inclined to not make any other changes ATM.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1350: if
(video_frame_provider_client_) {
On 2016/07/29 19:03:16, watk wrote:
> no braces

Done.

[watk, 2016-08-01 22:30:04]

lgtm

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.h (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:42: void
BindToLoop(int32_t stream_id,
On 2016/08/01 22:05:57, ThomasGuilbert wrote:
> On 2016/07/29 19:03:16, watk wrote:
> > tangential: we should rename this at some point to BindToTaskRunner
> 
> Done.

Thanks for doing it in this CL :)

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:44:
scoped_refptr<base::SingleThreadTaskRunner> loop);
s/loop/task_runner

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:64:
scoped_refptr<base::SingleThreadTaskRunner> loop_;
s/loop_/task_runner_/ (and docs above)

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1328:
stream_texture_proxy_->BindToLoop(stream_id_, frame_received_cb,
On 2016/08/01 22:05:57, ThomasGuilbert wrote:
> On 2016/07/29 19:03:16, watk wrote:
> > Same question as above. Is it thread safe to be accessing
> stream_texture_proxy_
> > and stream_id_ without locking?
> 
> I have not gone through the mental exercise of proving the thread safety/lack
> thereof. I think that while it is theoretically not thread safe, in practice
it
> is not causing any problems.
> 
> As I mentioned above, I am inclined to not make any other changes ATM.

sgtm with leaving as is

https://codereview.chromium.org/2192823004/diff/60001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.h (right):

https://codereview.chromium.org/2192823004/diff/60001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:44:
scoped_refptr<base::SingleThreadTaskRunner> loop);
s/loop/task_runner/

[tguilbert, 2016-08-01 23:03:18]

Thanks for the review!

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.h (right):

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:44:
scoped_refptr<base::SingleThreadTaskRunner> loop);
On 2016/08/01 22:30:03, watk wrote:
> s/loop/task_runner

Done.

https://codereview.chromium.org/2192823004/diff/40001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:64:
scoped_refptr<base::SingleThreadTaskRunner> loop_;
On 2016/08/01 22:30:03, watk wrote:
> s/loop_/task_runner_/ (and docs above)

Done.

https://codereview.chromium.org/2192823004/diff/60001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory.h (right):

https://codereview.chromium.org/2192823004/diff/60001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory.h:44:
scoped_refptr<base::SingleThreadTaskRunner> loop);
On 2016/08/01 22:30:03, watk wrote:
> s/loop/task_runner/

Done.

[tguilbert, 2016-08-01 23:03:22]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-08-01 23:03:23]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2192823004/#ps80001
(title: "Addressing comments")

[commit-bot: I haz the power, 2016-08-01 23:03:41]

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-08-02 00:20:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-02 00:24:40]

Description was changed from

==========
Update StreamTextureProxy to accept a base::Closure

StreamTextureProxy (STP) is currently tightly coupled to the
VideoFrameProvider::Client (VFP::C). This forces classes that use STP to
be aware of the VFP::C, and forces them to include files from cc/layers.

This change updates STP to accept a custom callback to be triggered when
the STP's OnFrameAvailable() is signaled. It also moves
StreamTextureWrapper from media/blink to media/base/android, now that it
no longer needs to include cc/layers/video_frame_provider.h.

TEST= Made sure we could still use WMPA (opened an HLS stream on an
android device)
BUG=631178
==========

to

==========
Update StreamTextureProxy to accept a base::Closure

StreamTextureProxy (STP) is currently tightly coupled to the
VideoFrameProvider::Client (VFP::C). This forces classes that use STP to
be aware of the VFP::C, and forces them to include files from cc/layers.

This change updates STP to accept a custom callback to be triggered when
the STP's OnFrameAvailable() is signaled. It also moves
StreamTextureWrapper from media/blink to media/base/android, now that it
no longer needs to include cc/layers/video_frame_provider.h.

TEST= Made sure we could still use WMPA (opened an HLS stream on an
android device)
BUG=631178

Committed: https://crrev.com/f4d502c5ec011e3b5b3aea1669b1d6149d5b9aed
Cr-Commit-Position: refs/heads/master@{#409100}
==========

[commit-bot: I haz the power, 2016-08-02 00:24:41]

Patchset 5 (id:??) landed as
https://crrev.com/f4d502c5ec011e3b5b3aea1669b1d6149d5b9aed
Cr-Commit-Position: refs/heads/master@{#409100}