[Cast] Refactor/clean-up VideoReceiver to match AudioReceiver as closely as possible.

[Cast] Refactor/clean-up VideoReceiver to match AudioReceiver as closely as possible.

This change is a necessary first step in our future plans to: 1) Merge the implementations of AudioReceiver and VideoReceiver into a single common module of code; and 2) Delegate A/V sync, jitter, and playout clock skew correction to a module that will operate at a higher abstraction level.

Several bugs were automatically fixed. Also, updated/fixed unit tests, improved comments/documentation, and tiny related clean-ups (where relevant to this change).

R=hubbe@chromium.org

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

[miu, 2014-04-04 18:25:01]

hubbe: PTAL.  This should be an easy code review since it's pretty much taking
AudioDecoder/Receiver and s/Audio/Video/ to make VideoDecoder/Receiver.  As
discussed, this will make us well-positioned to merge the two into a single
module of code.

Also, I updated the class comments for Audio/VideoReceiver to document the
"target playout delay" design concept, per our discussion with Patrik yesterday.

[hubbe, 2014-04-07 18:40:16]

First set of comments, will review some things in more details next round.

https://codereview.chromium.org/225023010/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:40: audio_config.rtp_max_delay_ms *
kTypicalFramesPerSecond / 1000),
Seems simpler to just divide by kTypicalAudioFrameDurationMs

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_config.cc
File media/cast/cast_config.cc (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_config.cc#ne...
media/cast/cast_config.cc:10: // TODO(miu): Revisit code factoring of these
structs.
I don't understand this TODO.

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_receiver_impl.h
File media/cast/cast_receiver_impl.h (left):

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_receiver_imp...
media/cast/cast_receiver_impl.h:40: void UpdateTargetDelay(base::TimeDelta
target_delay_ms);
Why are we deleting this?

https://codereview.chromium.org/225023010/diff/1/media/cast/test/utility/in_p...
File media/cast/test/utility/in_process_receiver.h (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/test/utility/in_p...
media/cast/test/utility/in_process_receiver.h:102: bool is_continuous);
Document |is_continuous| (and perhaps the entire method)

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
File media/cast/video_receiver/video_decoder.h (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
media/cast/video_receiver/video_decoder.h:43: // monotonically-increasing by 1
for each successive call to this method.
This is not really how cast works/should work, perhaps update the comment?

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
File media/cast/video_receiver/video_decoder_unittest.cc (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
media/cast/video_receiver/video_decoder_unittest.cc:90:
++total_video_frames_feed_in_;
Don't we have some sort of atomic increment class/helper function?

[miu, 2014-04-08 00:59:40]

https://codereview.chromium.org/225023010/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_receiver.cc (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.cc:40: audio_config.rtp_max_delay_ms *
kTypicalFramesPerSecond / 1000),
On 2014/04/07 18:40:17, hubbe wrote:
> Seems simpler to just divide by kTypicalAudioFrameDurationMs

Oh, duh.  Yes, agreed.  Done.

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_config.cc
File media/cast/cast_config.cc (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_config.cc#ne...
media/cast/cast_config.cc:10: // TODO(miu): Revisit code factoring of these
structs.
On 2014/04/07 18:40:17, hubbe wrote:
> I don't understand this TODO.

Added detail.

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_receiver_impl.h
File media/cast/cast_receiver_impl.h (left):

https://codereview.chromium.org/225023010/diff/1/media/cast/cast_receiver_imp...
media/cast/cast_receiver_impl.h:40: void UpdateTargetDelay(base::TimeDelta
target_delay_ms);
On 2014/04/07 18:40:17, hubbe wrote:
> Why are we deleting this?

It's a dead code path (no caller).  I'm the owner (I think) of implementing this
later on, once Mikhail figures out how the dynamic target delay adjustment is
supposed to work.  So, I wanted to remove this until we actually know what we're
going to do.

https://codereview.chromium.org/225023010/diff/1/media/cast/test/utility/in_p...
File media/cast/test/utility/in_process_receiver.h (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/test/utility/in_p...
media/cast/test/utility/in_process_receiver.h:102: bool is_continuous);
On 2014/04/07 18:40:17, hubbe wrote:
> Document |is_continuous| (and perhaps the entire method)

Done.  But, rather than duplicate comments, I've pointed the reader to
cast_receiver.h.

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
File media/cast/video_receiver/video_decoder.h (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
media/cast/video_receiver/video_decoder.h:43: // monotonically-increasing by 1
for each successive call to this method.
On 2014/04/07 18:40:17, hubbe wrote:
> This is not really how cast works/should work, perhaps update the comment?

As discussed, it is how it works; but probably not how it should work. 
Regardless, this refactoring didn't change the behavior, so it's okay to proceed
and we'll revisit this issue later.

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
File media/cast/video_receiver/video_decoder_unittest.cc (right):

https://codereview.chromium.org/225023010/diff/1/media/cast/video_receiver/vi...
media/cast/video_receiver/video_decoder_unittest.cc:90:
++total_video_frames_feed_in_;
On 2014/04/07 18:40:17, hubbe wrote:
> Don't we have some sort of atomic increment class/helper function?

No.  There's base::Atomic32, but the comments at the top of the file explicitly
say "know what you're doing" and "use a mutex if you don't need performance." 
base::AtomicRefCount is also close, but is essentially a semaphore and can only
tell you if it's value is one or zero.

[hubbe, 2014-04-08 16:41:23]

lgtm

[hubbe, 2014-04-08 16:42:05]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-04-08 16:42:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/225023010/20001

[commit-bot: I haz the power, 2014-04-08 16:42:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-08 16:42:46]

Failed to apply patch for media/cast/test/utility/in_process_receiver.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/cast/test/utility/in_process_receiver.cc
  Hunk #1 FAILED at 120.
  1 out of 1 hunk FAILED -- saving rejects to file
media/cast/test/utility/in_process_receiver.cc.rej

Patch:       media/cast/test/utility/in_process_receiver.cc
Index: media/cast/test/utility/in_process_receiver.cc
diff --git a/media/cast/test/utility/in_process_receiver.cc
b/media/cast/test/utility/in_process_receiver.cc
index
2b67666e7388727603e3dd60324b0fbc2666dbcf..4bc9e60c599e51b2fe05923ec7b81d23e109be2b
100644
--- a/media/cast/test/utility/in_process_receiver.cc
+++ b/media/cast/test/utility/in_process_receiver.cc
@@ -120,9 +120,13 @@ void InProcessReceiver::GotAudioFrame(scoped_ptr<AudioBus>
audio_frame,
 
 void InProcessReceiver::GotVideoFrame(
     const scoped_refptr<VideoFrame>& video_frame,
-    const base::TimeTicks& render_time) {
+    const base::TimeTicks& playout_time,
+    bool is_continuous) {
   DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
-  OnVideoFrame(video_frame, render_time);
+  if (video_frame) {
+    // TODO(miu): Pass |is_continuous| flag.
+    OnVideoFrame(video_frame, playout_time);
+  }
   PullNextVideoFrame();
 }

[miu, 2014-04-08 18:00:35]

Committed patchset #3 manually as r262449 (presubmit successful).

[miu, 2014-04-08 18:01:24]

FYI--Just dcommitted (bypassing the CQ) because this issue is blocking multiple
team members.  All trybot results were green, so we should be good to go.