Define Stream base classes

Define Stream base classes

BUG=webrtc:4690
Defined classes Stream, SendStream and ReceiveStream. Inherited existing stream classes from either SendStream or ReceiveStream.
This is a step towards having a Transport associated with streams instead of a Call. It will allow a lot of code in the Call to be media type agnostic.

R=henrika@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/cd6702282a49448adda470934f4bd9e6181cab22

[Jelena, 2015-07-13 13:59:04]

jmarusic@webrtc.org changed reviewers:
+ pbos@webrtc.org

[Jelena, 2015-07-13 14:03:37]

jmarusic@webrtc.org changed reviewers:
- pbos@webrtc.org

[Jelena, 2015-07-14 09:48:47]

jmarusic@webrtc.org changed reviewers:
+ pbos@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

[Jelena, 2015-07-14 10:57:20]

Patchset #1 (id:1) has been deleted

[Jelena, 2015-07-14 10:57:29]

Patchset #1 (id:2) has been deleted

[Jelena, 2015-07-14 10:57:41]

Patchset #1 (id:30001) has been deleted

[Jelena, 2015-07-14 10:58:46]

Please review.

[pbos-webrtc, 2015-07-14 14:50:13]

pbos@webrtc.org changed reviewers:
+ stefan@webrtc.org

[pbos-webrtc, 2015-07-14 14:50:14]

Overall looks good, adding stefan@ to get another pair of eyes on this as well.

https://codereview.webrtc.org/1226123005/diff/50001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.h (right):

https://codereview.webrtc.org/1226123005/diff/50001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.h:45: // webrtc::AudioReceiveStream
implementation.
Imo. you don't need this comment and the ones below.

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/stream.h
File webrtc/stream.h (right):

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/stream.h#newcode40
webrtc/stream.h:40: // Common base class for send streams.
Imo. you can remove this one, I don't think the readability is worth it, and I
think it's more confusing since it has no members.

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/video/audio_receiv...
File webrtc/video/audio_receive_stream.h (right):

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/video/audio_receiv...
webrtc/video/audio_receive_stream.h:29: // webrtc::ReceiveStream implementation.
Same w/r/t comments, imo. remove.

[Jelena, 2015-07-15 11:12:03]

https://codereview.webrtc.org/1226123005/diff/50001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.h (right):

https://codereview.webrtc.org/1226123005/diff/50001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.h:45: // webrtc::AudioReceiveStream
implementation.
On 2015/07/14 14:50:14, pbos-webrtc wrote:
> Imo. you don't need this comment and the ones below.

I actually like to have method overrides grouped and marked like this. Imo, it
makes the header easier to navigate.
Also, I have seen this practice a lot in our code base. So, if you don't mind
terribly, I would like to keep the comments, ok?

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/stream.h
File webrtc/stream.h (right):

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/stream.h#newcode40
webrtc/stream.h:40: // Common base class for send streams.
On 2015/07/14 14:50:14, pbos-webrtc wrote:
> Imo. you can remove this one, I don't think the readability is worth it, and I
> think it's more confusing since it has no members.

I prefer to keep this empty class to represent a send stream concept and provide
flexibility to add functionalities in the future. I will make sure to document
this intent. Check in the next patch how you like it, ok?

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/video/audio_receiv...
File webrtc/video/audio_receive_stream.h (right):

https://codereview.webrtc.org/1226123005/diff/50001/webrtc/video/audio_receiv...
webrtc/video/audio_receive_stream.h:29: // webrtc::ReceiveStream implementation.
On 2015/07/14 14:50:14, pbos-webrtc wrote:
> Same w/r/t comments, imo. remove.

Acknowledged.

[stefan-webrtc, 2015-07-15 11:48:46]

I see no issues with this.

https://codereview.webrtc.org/1226123005/diff/70001/webrtc/stream.h
File webrtc/stream.h (right):

https://codereview.webrtc.org/1226123005/diff/70001/webrtc/stream.h#newcode26
webrtc/stream.h:26: virtual void Stop() = 0;
What are Start and Stop supposed to be used for? I would like all methods in
this interface to be well commented.

[Jelena, 2015-07-15 12:38:55]

Please lgtm if no more issues.

[pbos-webrtc, 2015-07-15 13:06:53]

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h
File webrtc/stream.h (right):

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h#newcode26
webrtc/stream.h:26: // When a stream is active, it can receive, process and
deliver samples.
s/samples/packets/

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h#newcode31
webrtc/stream.h:31: // Called to notify that network state has changed.
Used to stop encoding and sending data while the network is down.

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h#newcode49
webrtc/stream.h:49: // for future functionality.
Remove the "serves as a placeholder..", we don't add placeholders if there's no
clear functionality that will definitely be added.

Imo, do "typedef Stream SendStream;" to show that it's just for symmetry with
ReceiveStream. If we want to add stuff to it, we'll do it then.

[Jelena, 2015-07-15 14:21:45]

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h
File webrtc/stream.h (right):

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h#newcode26
webrtc/stream.h:26: // When a stream is active, it can receive, process and
deliver samples.
On 2015/07/15 13:06:53, pbos-webrtc wrote:
> s/samples/packets/

Done.

https://codereview.webrtc.org/1226123005/diff/90001/webrtc/stream.h#newcode49
webrtc/stream.h:49: // for future functionality.
On 2015/07/15 13:06:53, pbos-webrtc wrote:
> Remove the "serves as a placeholder..", we don't add placeholders if there's
no
> clear functionality that will definitely be added.
> 
> Imo, do "typedef Stream SendStream;" to show that it's just for symmetry with
> ReceiveStream. If we want to add stuff to it, we'll do it then.

I agree that at a first glance, having an empty class looks unnecessary.
However, having a class instead of a typedef here, tells both a human and a
compiler that SendStream is -not- the same as Stream. Consider this:

VideoReceiveStream* receive_stream = ...;
SendStream* send_stream = static_cast<SendStream*>(receive_stream);

If SendStream is defined like this:
class SendStream : public Stream {};
the compiler catches this error.
If SendStream is defined like this:
typedef Stream SendStream;
the erroneous code gets compiled and possibly a strange bug is introduced.

[pbos-webrtc, 2015-07-15 14:51:52]

sgtm, lgtm

[stefan-webrtc, 2015-07-15 14:58:33]

lgtm

[Jelena, 2015-07-15 15:07:26]

jmarusic@webrtc.org changed reviewers:
+ henrika@webrtc.org

[Jelena, 2015-07-15 15:10:00]

Henrik, I need your lgtm, as you are the only owner at the top webrtc directory
that is currently at the office.

[henrika_webrtc, 2015-07-15 15:12:31]

LGTM. Don't know the details here but trust the other reviewers ;-)

[Jelena, 2015-07-16 07:30:29]

Committed patchset #4 (id:110001) manually as
cd6702282a49448adda470934f4bd9e6181cab22 (presubmit successful).