Issue 168933002: Cast: Implemented relative RTP timestamp in EncodingEventSubscriber.

Issue 168933002: Cast: Implemented relative RTP timestamp in EncodingEventSubscriber. (Closed)

Created:
6 years, 10 months ago by imcheng

Modified:
6 years, 10 months ago

Reviewers:
Alpha Left Google

CC:
chromium-reviews, hclam+watch_chromium.org, mikhal+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, pwestin+watch_google.com, feature-media-reviews_chromium.org, jshin+watch_chromium.org, hubbe+watch_chromium.org, miu+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Cast: Implemented relative RTP timestamp in EncodingEventSubscriber. - RTP timestamps in the maps of EncodingEventSubscribers (map keys, rtp timestamp fields in protos) are now relative to the first RTP timestamp seen since last Reset(). - The two GetXXXEventsAndReset() functions have been combined into one. In addition, it will now return the first RTP timestamp seen since the last Reset(). It will also reset the first RTP timestamp seen. - There are two motivations for this change: -- 1. RTP timestamps wrap around easily - around every 12 hours or so. If we do not use relative RTP timestamps, the ordering in the map, as well as general order of the events by frame, will be messed up. -- 2. Using relative values will be more compact in variable-length encoding of the RTP timestamp fields. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251973

Patch Set 1 #

Total comments: 2

Patch Set 2 : Addressed Alpha's comment #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+182 lines, -75 lines)			Patch
M	media/cast/logging/encoding_event_subscriber.h	View	1	3 chunks	+25 lines, -7 lines	0 comments	Download
M	media/cast/logging/encoding_event_subscriber.cc	View		5 chunks	+41 lines, -20 lines	0 comments	Download
M	media/cast/logging/encoding_event_subscriber_unittest.cc	View		19 chunks	+116 lines, -48 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Alpha Left Google

LGTM with one nit. https://codereview.chromium.org/168933002/diff/1/media/cast/logging/encoding_event_subscriber.h File media/cast/logging/encoding_event_subscriber.h (right): https://codereview.chromium.org/168933002/diff/1/media/cast/logging/encoding_event_subscriber.h#newcode89 media/cast/logging/encoding_event_subscriber.h:89: // Set to RTP timestamp ...

6 years, 10 months ago (2014-02-18 21:54:27 UTC) #2

imcheng

https://codereview.chromium.org/168933002/diff/1/media/cast/logging/encoding_event_subscriber.h File media/cast/logging/encoding_event_subscriber.h (right): https://codereview.chromium.org/168933002/diff/1/media/cast/logging/encoding_event_subscriber.h#newcode89 media/cast/logging/encoding_event_subscriber.h:89: // Set to RTP timestamp of first event encountered ...

6 years, 10 months ago (2014-02-19 01:12:27 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/168933002/80001

6 years, 10 months ago (2014-02-19 01:13:34 UTC) #5

Message was sent while issue was closed.

Change committed as 251973

Expand Messages | Collapse Messages