Convenience functions to convert RtcEvents to webrtc types.

Creates convenience functions to convert stored RtcEvents to the original webrtc types. Moves all parsing of RtcEventLog to a separate class.

[terelius, 2015-08-14 12:41:59]

terelius@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, ivoc@webrtc.org, stefan@webrtc.org

[terelius, 2015-08-14 12:47:31]

I have not added unit tests in this CL because the existing unit tests for
RTcEventLog has changed a lot lately. I believe it will be cleaner to add unit
tests after https://codereview.webrtc.org/1257163003/
has landed.

[hlundin-webrtc, 2015-08-18 09:22:59]

Nice. But see comments.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log.cc
File webrtc/video/rtc_event_log.cc (right):

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log.cc...
webrtc/video/rtc_event_log.cc:13: #include <cstring>
string.h

We tend to use the C header file for C libraries rather than the C++ one.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
File webrtc/video/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:63: DCHECK(event.has_timestamp_us());
Why DCHECK and not CHECK? What happens if timestamp is missing, but we ask for
it anyway?

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:69: DCHECK(event.has_type());
Same as above.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:84: // Get direction of packet
End comments with a period. Here and below.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:104: if (rtp_packet.header().size() <=
IP_PACKET_SIZE) {
Simply CHECK_LE(rtp_packet.header().size(), IP_PACKET_SIZE)
and omit the if statement.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:105: memcpy(header,
rtp_packet.header().data(), rtp_packet.header().size());
#include <string.h>

(Include what you use.)

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:108: // TODO(terelius): Should we assert
that we never reach this code?
With my comment above, this code is not needed.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:110: return false;
Also with my comment above, this function can only return true => return void
instead.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:113: // The packet must have space for at
least IP_PACKET_SIZE bytes
End with .

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:144: // TODO(terelius): Should we assert
that we never reach this code?
Yes. And return void.

[terelius, 2015-08-18 16:06:51]

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log.cc
File webrtc/video/rtc_event_log.cc (right):

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log.cc...
webrtc/video/rtc_event_log.cc:13: #include <cstring>
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> string.h
> 
> We tend to use the C header file for C libraries rather than the C++ one.

It should actually be <string>. The C string library should be included in
another file. Thanks.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
File webrtc/video/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:63: DCHECK(event.has_timestamp_us());
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> Why DCHECK and not CHECK? What happens if timestamp is missing, but we ask for
> it anyway?

It will return the default value and the program continues. In this case it will
not cause problems with dereference null-pointers. You are probably right that
CHECK is better though. I'll change that.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:69: DCHECK(event.has_type());
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> Same as above.

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:84: // Get direction of packet
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> End comments with a period. Here and below.

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:104: if (rtp_packet.header().size() <=
IP_PACKET_SIZE) {
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> Simply CHECK_LE(rtp_packet.header().size(), IP_PACKET_SIZE)
> and omit the if statement.

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:105: memcpy(header,
rtp_packet.header().data(), rtp_packet.header().size());
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> #include <string.h>
> 
> (Include what you use.)

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:108: // TODO(terelius): Should we assert
that we never reach this code?
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> With my comment above, this code is not needed.

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:110: return false;
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> Also with my comment above, this function can only return true => return void
> instead.

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:113: // The packet must have space for at
least IP_PACKET_SIZE bytes
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> End with .

Done.

https://codereview.webrtc.org/1295753003/diff/1/webrtc/video/rtc_event_log_pa...
webrtc/video/rtc_event_log_parser.cc:144: // TODO(terelius): Should we assert
that we never reach this code?
On 2015/08/18 09:22:59, hlundin-webrtc wrote:
> Yes. And return void.

Done.

[ivoc, 2015-08-25 15:25:55]

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log.cc (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log.cc:13: #include <string>
I don't think this include is needed here, as it is already included in
rtc_event_log.h.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:105: memcpy(header,
rtp_packet.header().data(), rtp_packet.header().size());
Another option here would be to allocate the data here, and pass ownership to
the caller. You could do this by changing the header argument to uint8_t** type,
if it's set to nullptr, don't allocate a header, otherwise, allocate from here
and fill the buffer.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:137: rtcp_packet.packet_data().size());
Same here.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:157: switch (receiver_config.rtcp_mode()) {
Would be nice to refactor this into a seperate conversion function, like the
other enum conversion function.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.h (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:22: #include
"webrtc/video/rtc_event_log.pb.h"
I wonder if there is a way to hide the protobuf internals completely. Maybe it
could be enough to add these two functions: 
int GetNumberOfEvents(EventStream&) 
and
void GetEvent(EventStream&, int index, Event*)
What do you think?

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:33: static MediaType
GetRuntimeMediaType(rtclog::MediaType media_type);
Does this function need to be public? Might be better to move it to an anonymous
namespace in the .cc file.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:50: // Returns false if the stored header is
larger than IP_PACKET_SIZE.
Since you changed the return type, the comments should be updated as well.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:62: // Returns false if the stored packet is
larger than IP_PACKET_SIZE
Same here.

[hlundin-webrtc, 2015-08-28 10:37:16]

Looks good, but please, address Ivo's comments.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log.cc (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log.cc:13: #include <string>
On 2015/08/25 15:25:55, ivoc wrote:
> I don't think this include is needed here, as it is already included in
> rtc_event_log.h. 

True. If it cannot be avoided in the .h file, it should not be included in the
.cc file.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.h (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:33: static MediaType
GetRuntimeMediaType(rtclog::MediaType media_type);
On 2015/08/25 15:25:55, ivoc wrote:
> Does this function need to be public? Might be better to move it to an
anonymous
> namespace in the .cc file.

Acknowledged.

[terelius, 2015-09-23 11:41:17]

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log.cc (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log.cc:13: #include <string>
On 2015/08/25 15:25:55, ivoc wrote:
> I don't think this include is needed here, as it is already included in
> rtc_event_log.h. 

Done.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log.cc:13: #include <string>
On 2015/08/28 10:37:16, hlundin-webrtc wrote:
> On 2015/08/25 15:25:55, ivoc wrote:
> > I don't think this include is needed here, as it is already included in
> > rtc_event_log.h. 
> 
> True. If it cannot be avoided in the .h file, it should not be included in the
> .cc file.

Acknowledged.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:105: memcpy(header,
rtp_packet.header().data(), rtp_packet.header().size());
On 2015/08/25 15:25:55, ivoc wrote:
> Another option here would be to allocate the data here, and pass ownership to
> the caller. You could do this by changing the header argument to uint8_t**
type,
> if it's set to nullptr, don't allocate a header, otherwise, allocate from here
> and fill the buffer. 

The problem is that doing that would force a new allocation for each packet,
even if the application only want to check the header for an absolute send time.
I prefer doing it this way since ownership is more explicit and the same buffer
can be reused for multiple headers.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:137: rtcp_packet.packet_data().size());
On 2015/08/25 15:25:55, ivoc wrote:
> Same here.

See above.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:157: switch (receiver_config.rtcp_mode()) {
On 2015/08/25 15:25:55, ivoc wrote:
> Would be nice to refactor this into a seperate conversion function, like the
> other enum conversion function.

Done.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.h (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:22: #include
"webrtc/video/rtc_event_log.pb.h"
On 2015/08/25 15:25:55, ivoc wrote:
> I wonder if there is a way to hide the protobuf internals completely. Maybe it
> could be enough to add these two functions: 
> int GetNumberOfEvents(EventStream&) 
> and
> void GetEvent(EventStream&, int index, Event*)
> What do you think?

I agree that it would be very nice to hide the protobuf internals. I have added
the functions you suggested, but  it does not completely hide the protobuf. We
are still manipulating the protobuf data structures and in particular, we still
have to use the protobuf EventType constants, e.g. rtclog::Event::RTP_EVENT

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:33: static MediaType
GetRuntimeMediaType(rtclog::MediaType media_type);
On 2015/08/25 15:25:55, ivoc wrote:
> Does this function need to be public? Might be better to move it to an
anonymous
> namespace in the .cc file.

This function is used in the unit test. (In fact it was moved here from the unit
test.)

One solution would be to rewrite the unit test using the parser code, but then a
lot of the verifications will be done with CHECK rather than ASSERT/EXPECT. I am
open to suggestions.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:50: // Returns false if the stored header is
larger than IP_PACKET_SIZE.
On 2015/08/25 15:25:55, ivoc wrote:
> Since you changed the return type, the comments should be updated as well.

Done.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:62: // Returns false if the stored packet is
larger than IP_PACKET_SIZE
On 2015/08/25 15:25:55, ivoc wrote:
> Same here.

Done.

[ivoc, 2015-09-25 14:16:34]

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.cc (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.cc:105: memcpy(header,
rtp_packet.header().data(), rtp_packet.header().size());
On 2015/09/23 11:41:17, terelius wrote:
> On 2015/08/25 15:25:55, ivoc wrote:
> > Another option here would be to allocate the data here, and pass ownership
to
> > the caller. You could do this by changing the header argument to uint8_t**
> type,
> > if it's set to nullptr, don't allocate a header, otherwise, allocate from
here
> > and fill the buffer. 
> 
> The problem is that doing that would force a new allocation for each packet,
> even if the application only want to check the header for an absolute send
time.
> I prefer doing it this way since ownership is more explicit and the same
buffer
> can be reused for multiple headers.

Acknowledged.

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
File webrtc/video/rtc_event_log_parser.h (right):

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:22: #include
"webrtc/video/rtc_event_log.pb.h"
On 2015/09/23 11:41:17, terelius wrote:
> On 2015/08/25 15:25:55, ivoc wrote:
> > I wonder if there is a way to hide the protobuf internals completely. Maybe
it
> > could be enough to add these two functions: 
> > int GetNumberOfEvents(EventStream&) 
> > and
> > void GetEvent(EventStream&, int index, Event*)
> > What do you think?
> 
> I agree that it would be very nice to hide the protobuf internals. I have
added
> the functions you suggested, but  it does not completely hide the protobuf. We
> are still manipulating the protobuf data structures and in particular, we
still
> have to use the protobuf EventType constants, e.g. rtclog::Event::RTP_EVENT
> 

I think that it is possible to completely hide the internals if we really want
to. Those protobuf enums can be translated to different ones that are defined in
this header, and the Event and EventStream themselves can be just forward
declared in the header (which works fine as long as the "user" doesn't need to
call any functions on it, and they only appear as references or pointers in the
header). I'm not sure if it's worth it though, what do you think?

https://codereview.webrtc.org/1295753003/diff/20001/webrtc/video/rtc_event_lo...
webrtc/video/rtc_event_log_parser.h:33: static MediaType
GetRuntimeMediaType(rtclog::MediaType media_type);
On 2015/09/23 11:41:17, terelius wrote:
> On 2015/08/25 15:25:55, ivoc wrote:
> > Does this function need to be public? Might be better to move it to an
> anonymous
> > namespace in the .cc file.
> 
> This function is used in the unit test. (In fact it was moved here from the
unit
> test.)
> 
> One solution would be to rewrite the unit test using the parser code, but then
a
> lot of the verifications will be done with CHECK rather than ASSERT/EXPECT. I
am
> open to suggestions.

Okay, good point. I guess it's fine to leave it here then.

[stefan-webrtc, 2016-05-06 10:54:35]

stefan@webrtc.org changed reviewers:
- stefan@webrtc.org