Cast: Removed unnecessary ref counters.

Cast: Removed unnecessary ref counters.

AudioDecoder and VideoEncoder objects inherited RefCountedThreadSafe
for no valid reason. AudioDecoder need no Refcounter or Weak ptr.
The VideoEncoder can accomplish the same using a weak ptr.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236887

[pwestin, 2013-11-15 17:44:34]

Please review

[Alpha Left Google, 2013-11-15 19:40:38]

https://codereview.chromium.org/74133002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_decoder.cc (right):

https://codereview.chromium.org/74133002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_decoder.cc:55: if
(!base::subtle::NoBarrier_Load(&have_received_packets_)) return false;
We very rarely use atomicops. What is the reason that we need them here? I
though this object is non-threadsafe so what are the threads that use this
object?

[pwestin, 2013-11-15 19:48:06]

Answer to question

https://codereview.chromium.org/74133002/diff/30001/media/cast/audio_receiver...
File media/cast/audio_receiver/audio_decoder.cc (right):

https://codereview.chromium.org/74133002/diff/30001/media/cast/audio_receiver...
media/cast/audio_receiver/audio_decoder.cc:55: if
(!base::subtle::NoBarrier_Load(&have_received_packets_)) return false;
On 2013/11/15 19:40:39, Alpha wrote:
> We very rarely use atomicops. What is the reason that we need them here? I
> though this object is non-threadsafe so what are the threads that use this
> object?

I was looking for other options but did not find any simpler approach. 

GetRawAudioFrame is called from the cast audio decoder thread and
IncomingParsedRtpPacket is called from the Main cast thread

[Alpha Left Google, 2013-11-15 19:52:09]

> I was looking for other options but did not find any simpler approach. 
> 
> GetRawAudioFrame is called from the cast audio decoder thread and
> IncomingParsedRtpPacket is called from the Main cast thread

What about put AudioDecoder on the audio decoder thread and then
IncomingParsedRtpPacket is called on the AudioDecoder as well? You can post task
to audio decoder thread to call IncomingParsedRtpPacket(), no?

[pwestin, 2013-11-15 19:58:22]

On 2013/11/15 19:52:09, Alpha wrote:
> > I was looking for other options but did not find any simpler approach. 
> > 
> > GetRawAudioFrame is called from the cast audio decoder thread and
> > IncomingParsedRtpPacket is called from the Main cast thread
> 
> What about put AudioDecoder on the audio decoder thread and then
> IncomingParsedRtpPacket is called on the AudioDecoder as well? You can post
task
> to audio decoder thread to call IncomingParsedRtpPacket(), no?

Yes we could to that at the cost of another thread context switch. WDYT is it
worth it?

[Alpha Left Google, 2013-11-15 20:01:08]

> Yes we could to that at the cost of another thread context switch. WDYT is it
> worth it?

That's what we do already for video isn't it? I think it's best to be
consistent. Then if performance actually matters we can change them
(AudioEncoder and VideoEncoder) both.

[pwestin, 2013-11-15 20:49:54]

On 2013/11/15 20:01:08, Alpha wrote:
> > Yes we could to that at the cost of another thread context switch. WDYT is
it
> > worth it?
> 
> That's what we do already for video isn't it? I think it's best to be
> consistent. Then if performance actually matters we can change them
> (AudioEncoder and VideoEncoder) both.

Doooh looking into this at a deeper level; we can't do this due to the way
webrtc is different that our video case. To do this we either have to duplicate
the packet :( or ripple the closure callback through the RTP parser for this use
case only (that would not be good)

[Alpha Left Google, 2013-11-18 22:01:13]

https://codereview.chromium.org/74133002/diff/120001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_decoder.h (right):

https://codereview.chromium.org/74133002/diff/120001/media/cast/audio_receive...
media/cast/audio_receiver/audio_decoder.h:46: base::subtle::Atomic32
have_received_packets_;
Is there a difference in terms of performance using lock or atomicops? I would
prefer using locks to atomicops.

[pwestin, 2013-11-22 19:16:28]

PTAL

https://codereview.chromium.org/74133002/diff/120001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_decoder.h (right):

https://codereview.chromium.org/74133002/diff/120001/media/cast/audio_receive...
media/cast/audio_receiver/audio_decoder.h:46: base::subtle::Atomic32
have_received_packets_;
On 2013/11/18 22:01:14, Alpha wrote:
> Is there a difference in terms of performance using lock or atomicops? I would
> prefer using locks to atomicops.

I doubt there is a measurable difference for this use case.
Changed to Lock

[Alpha Left Google, 2013-11-22 20:03:52]

https://codereview.chromium.org/74133002/diff/190001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_decoder.cc (right):

https://codereview.chromium.org/74133002/diff/190001/media/cast/audio_receive...
media/cast/audio_receiver/audio_decoder.cc:55: // Get a local copy under lock.
Do you expect to have a race condition using the lock this way? Or this is a
race condition that we do not care?

[pwestin, 2013-11-22 21:16:36]

https://codereview.chromium.org/74133002/diff/190001/media/cast/audio_receive...
File media/cast/audio_receiver/audio_decoder.cc (right):

https://codereview.chromium.org/74133002/diff/190001/media/cast/audio_receive...
media/cast/audio_receiver/audio_decoder.cc:55: // Get a local copy under lock.
On 2013/11/22 20:03:53, Alpha wrote:
> Do you expect to have a race condition using the lock this way? Or this is a
> race condition that we do not care?

We don't care about the race where a thread ask for audio and the packet come in
at the same time. We will get the data in the next get request.

[Alpha Left Google, 2013-11-22 21:17:52]

On 2013/11/22 21:16:36, pwestin wrote:
>
https://codereview.chromium.org/74133002/diff/190001/media/cast/audio_receive...
> File media/cast/audio_receiver/audio_decoder.cc (right):
> 
>
https://codereview.chromium.org/74133002/diff/190001/media/cast/audio_receive...
> media/cast/audio_receiver/audio_decoder.cc:55: // Get a local copy under lock.
> On 2013/11/22 20:03:53, Alpha wrote:
> > Do you expect to have a race condition using the lock this way? Or this is a
> > race condition that we do not care?
> 
> We don't care about the race where a thread ask for audio and the packet come
in
> at the same time. We will get the data in the next get request.

Can you comment that in the code please?

[pwestin, 2013-11-22 21:51:32]

Done

[Alpha Left Google, 2013-11-22 21:53:55]

LGTM.

[commit-bot: I haz the power, 2013-11-22 22:37:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/74133002/340002

[commit-bot: I haz the power, 2013-11-23 00:59:17]

Change committed as 236887