Remove implicit conversions from scoped_refptr to T* in media/

Remove implicit conversions from scoped_refptr to T* in media/

This patch was generated by running the rewrite_scoped_refptr clang tool
on a Linux build.

BUG=110610
Committed: https://crrev.com/d048bd27d5abdc271b10df7d3b434f9b9dd767d3
Cr-Commit-Position: refs/heads/master@{#292082}

[dcheng, 2014-08-25 19:01:43]

dcheng@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2014-08-25 21:26:40]

Most of these are conditionals or DCHECKs. A bunch of other uses are in tests,
which I didn't examine closely. I found a couple of concerning uses in
production code

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
File media/cast/test/utility/udp_proxy.cc (right):

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
media/cast/test/utility/udp_proxy.cc:695: result = socket_->SendTo(buf.get(),
I believe this is a pointer to a RefCounted object, so it was fine as-is?

I'm not sure whether Cast actually uses this code, but this is what I'm looking
at.
https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
media/cast/test/utility/udp_proxy.cc:776: recv_buf.get(),
ditto

https://codereview.chromium.org/506683002/diff/1/media/filters/vpx_video_deco...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/506683002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:270: memory_pool_.get())) {
This parameter is "Callback's private data".
I didn't verify that the member outlives this use.

[dcheng, 2014-08-25 21:33:25]

Sorry, I really should have split media/cast into a separate review to make this
more manageable. I apologize =(

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
File media/cast/test/utility/udp_proxy.cc (right):

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
media/cast/test/utility/udp_proxy.cc:695: result = socket_->SendTo(buf.get(),
On 2014/08/25 21:26:39, ddorwin wrote:
> I believe this is a pointer to a RefCounted object, so it was fine as-is?
> 
> I'm not sure whether Cast actually uses this code, but this is what I'm
looking
> at.
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...

Right, but before it was depending on the conversion operator to be invoked.
Since we're removing the implicit conversion, we need to force an explicit
conversion with .get().

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
media/cast/test/utility/udp_proxy.cc:776: recv_buf.get(),
On 2014/08/25 21:26:40, ddorwin wrote:
> ditto

See above.

https://codereview.chromium.org/506683002/diff/1/media/filters/vpx_video_deco...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/506683002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:270: memory_pool_.get())) {
On 2014/08/25 21:26:40, ddorwin wrote:
> This parameter is "Callback's private data".
> I didn't verify that the member outlives this use.

It's worth noting this was already using the implicit conversion operator
before. Making this explicit doesn't make it any less memory-safe--the code in
question must be keeping it alive some other way, or this is an error (since the
callback parameter is a void*).

[ddorwin, 2014-08-25 21:39:43]

I understand that this doesn't make things worse, but if the point of removing
the implicit conversions is to detect bugs, then the owners of these files
should probably be taking a close look at these specific cases as they are
converted.

I'm happy to LG this, but I think we should make sure we have the right people
checking these (either here, via email, or in a bug). WDYT?

[dcheng, 2014-08-25 21:44:27]

dcheng@chromium.org changed reviewers:
+ hubbe@chromium.org, xhwang@chromium.org

[dcheng, 2014-08-25 21:44:27]

On 2014/08/25 at 21:39:43, ddorwin wrote:
> I understand that this doesn't make things worse, but if the point of removing
the implicit conversions is to detect bugs, then the owners of these files
should probably be taking a close look at these specific cases as they are
converted.
> 
> I'm happy to LG this, but I think we should make sure we have the right people
checking these (either here, via email, or in a bug). WDYT?

Sure. I've added who I think the appropriate reviewers are, but please correct
me if I'm wrong =)

+hubbe@chromium.org to verify that an explicit conversion from scoped_refptr<T>
to T* is safe in udp_proxy.cc.
+xhwang to verify that there are no lifetime issues in vpx_video_decoder.cc when
passing a ref counted object as the callback data.

[ddorwin, 2014-08-25 23:57:35]

ddorwin@chromium.org changed reviewers:
+ tomfinegan@chromium.org

[ddorwin, 2014-08-25 23:59:09]

On 2014/08/25 21:44:27, dcheng (OOO) wrote:
> On 2014/08/25 at 21:39:43, ddorwin wrote:
> > I understand that this doesn't make things worse, but if the point of
removing
> the implicit conversions is to detect bugs, then the owners of these files
> should probably be taking a close look at these specific cases as they are
> converted.
> > 
> > I'm happy to LG this, but I think we should make sure we have the right
people
> checking these (either here, via email, or in a bug). WDYT?
> 
> Sure. I've added who I think the appropriate reviewers are, but please correct
> me if I'm wrong =)
> 
> mailto:+hubbe@chromium.org to verify that an explicit conversion from
scoped_refptr<T>
> to T* is safe in udp_proxy.cc.
> +xhwang to verify that there are no lifetime issues in vpx_video_decoder.cc
when
> passing a ref counted object as the callback data.

tomfinegan: PTAL at media/filters/vpx_video_decoder.cc and my question there.

[Tom Finegan, 2014-08-26 00:26:11]

lgtm VpxVideoDecoder

https://codereview.chromium.org/506683002/diff/1/media/filters/vpx_video_deco...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/506683002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.cc:270: memory_pool_.get())) {
On 2014/08/25 21:33:25, dcheng (OOO) wrote:
> On 2014/08/25 21:26:40, ddorwin wrote:
> > This parameter is "Callback's private data".
> > I didn't verify that the member outlives this use.
> 
> It's worth noting this was already using the implicit conversion operator
> before. Making this explicit doesn't make it any less memory-safe--the code in
> question must be keeping it alive some other way, or this is an error (since
the
> callback parameter is a void*).

caveat: I did not write or review this part of VpxVideoDecoder, but...

Libvpx is the one using that void* for the static methods in VpxVideoDecoder's
MemoryPool class. VpxVideoDecoder is holding the reference for libvpx because
libvpx doesn't know anything about scoped_refptrs. VpxVideoDecoder is nulling it
out in CloseDecoder()(thus releasing the reference), but only after libvpx has
been shut down. This should be safe because after that point libvpx will no
longer be alive to call GetVP9FrameBuffer() or ReleaseVP9FrameBuffer().

Assuming that's what you're asking, anyway.

[hubbe, 2014-08-27 00:46:21]

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
File media/cast/test/utility/udp_proxy.cc (right):

https://codereview.chromium.org/506683002/diff/1/media/cast/test/utility/udp_...
media/cast/test/utility/udp_proxy.cc:695: result = socket_->SendTo(buf.get(),
On 2014/08/25 21:33:25, dcheng (OOO) wrote:
> On 2014/08/25 21:26:39, ddorwin wrote:
> > I believe this is a pointer to a RefCounted object, so it was fine as-is?
> > 
> > I'm not sure whether Cast actually uses this code, but this is what I'm
> looking
> > at.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...
> 
> Right, but before it was depending on the conversion operator to be invoked.
> Since we're removing the implicit conversion, we need to force an explicit
> conversion with .get().

Explicit conversion is fine here.
The buffers lifetime is (intentionally) maintained by the callback below.

[ddorwin, 2014-08-27 00:47:32]

LGTM with the two reviews above.

[dcheng, 2014-08-27 01:41:23]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-08-27 01:42:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/506683002/1

[commit-bot: I haz the power, 2014-08-27 02:25:31]

Committed patchset #1 (1) as 15a7c1cbb7a4d67a95b4e3c380b1a794e17806d1

[commit-bot: I haz the power, 2014-09-10 02:49:13]

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