Cast: Only ACK decodable frames

Cast: Only ACK decododable frames

Also, add an optimization to not decode frames if we are behind and multiple frames are currently decodable.

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

[imcheng, 2014-05-14 20:11:23]

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
File media/cast/framer/cast_message_builder.cc (right):

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
media/cast/framer/cast_message_builder.cc:35:
DCHECK_GE(static_cast<int32>(frame_id - last_acked_frame_id_), 0);
What guarantees this?

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
media/cast/framer/cast_message_builder.cc:79: frame_id = ack_queue_.front();
Do you need to do another pop_front()? Comment above states ack every other
frame.

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
media/cast/framer/cast_message_builder.cc:79: frame_id = ack_queue_.front();
Can ack_queue_ be empty at this point?

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

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.cc#...
media/cast/framer/framer.cc:23: &frame_id_map_,
I have limited context from just looking at this file, but I was wondering if it
makes sense to combine FrameIdMap with FrameList. It seems what we are doing
here is keeping the two objects "consistent" by modifying them similarly (e.g.
lookup FrameIdMap then use result to look up Framer, or erase from FrameIdMap
and then Framer), and it looks error prone this way.

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.cc#...
media/cast/framer/framer.cc:114: if (next_frame) {
*next_frame

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.h
File media/cast/framer/framer.h (right):

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.h#n...
media/cast/framer/framer.h:43: // Extracts a complete encoded frame - will only
return a complete continuous
From implementation it looks like this is not the case? If it went to the
NextVideoFrameAllowingSkippingFrames branch. Is this comment outdated?

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.h#n...
media/cast/framer/framer.h:48: bool* next_frame,
What does |next_frame| mean?

[hubbe, 2014-05-14 21:46:15]

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
File media/cast/framer/cast_message_builder.cc (right):

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
media/cast/framer/cast_message_builder.cc:35:
DCHECK_GE(static_cast<int32>(frame_id - last_acked_frame_id_), 0);
On 2014/05/14 20:11:24, imcheng1 wrote:
> What guarantees this?

Hopefully the cast_receiver does.
I put it in here because some of the logic below assumes it.

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
media/cast/framer/cast_message_builder.cc:79: frame_id = ack_queue_.front();
On 2014/05/14 20:11:24, imcheng1 wrote:
> Do you need to do another pop_front()? Comment above states ack every other
> frame.

What it means is that in slow-ack mode we ack delay acking like this:

We are asked to ack:
1     2     3     4     5     6     7
What we actually ack:
1           2           3           4

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/cast_messa...
media/cast/framer/cast_message_builder.cc:79: frame_id = ack_queue_.front();
On 2014/05/14 20:11:24, imcheng1 wrote:
> Can ack_queue_ be empty at this point?

No.
When we go into slow mode, we push one value on the queue.
After that we push one value every call and pull one value every other call. We
will have at least one value.

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

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.cc#...
media/cast/framer/framer.cc:23: &frame_id_map_,
On 2014/05/14 20:11:24, imcheng1 wrote:
> I have limited context from just looking at this file, but I was wondering if
it
> makes sense to combine FrameIdMap with FrameList. It seems what we are doing
> here is keeping the two objects "consistent" by modifying them similarly (e.g.
> lookup FrameIdMap then use result to look up Framer, or erase from FrameIdMap
> and then Framer), and it looks error prone this way.

That does seem like a good idea, but is out of scope of this CL.

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.cc#...
media/cast/framer/framer.cc:114: if (next_frame) {
On 2014/05/14 20:11:24, imcheng1 wrote:
> *next_frame

Done (so much for test coverage)

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.h
File media/cast/framer/framer.h (right):

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.h#n...
media/cast/framer/framer.h:43: // Extracts a complete encoded frame - will only
return a complete continuous
On 2014/05/14 20:11:24, imcheng1 wrote:
> From implementation it looks like this is not the case? If it went to the
> NextVideoFrameAllowingSkippingFrames branch. Is this comment outdated?

Done.

https://codereview.chromium.org/289483003/diff/1/media/cast/framer/framer.h#n...
media/cast/framer/framer.h:48: bool* next_frame,
On 2014/05/14 20:11:24, imcheng1 wrote:
> What does |next_frame| mean?

Done.

[imcheng, 2014-05-14 22:14:15]

lgtm

[hubbe, 2014-05-14 22:39:18]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:40:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/289483003/40001

[commit-bot: I haz the power, 2014-05-14 22:40:25]

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

[commit-bot: I haz the power, 2014-05-14 22:40:25]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[hubbe, 2014-05-15 19:04:53]

Reviewed by Derek, needs approval.

[miu, 2014-05-16 19:45:24]

https://codereview.chromium.org/289483003/diff/40001/media/cast/framer/framer...
File media/cast/framer/framer_unittest.cc (right):

https://codereview.chromium.org/289483003/diff/40001/media/cast/framer/framer...
media/cast/framer/framer_unittest.cc:38: bool multiple = false;
In all these tests, consider testing that "multiple" was set correctly.  ;-)

https://codereview.chromium.org/289483003/diff/40001/media/cast/video_receive...
File media/cast/video_receiver/video_receiver.cc (right):

https://codereview.chromium.org/289483003/diff/40001/media/cast/video_receive...
media/cast/video_receiver/video_receiver.cc:158: bool
have_multiple_complete_frames = false;
Please make these changes identically in AudioReceiver.  I promise I'll merge
the two shortly.  ;-)

[hubbe, 2014-05-16 20:50:58]

https://codereview.chromium.org/289483003/diff/40001/media/cast/framer/framer...
File media/cast/framer/framer_unittest.cc (right):

https://codereview.chromium.org/289483003/diff/40001/media/cast/framer/framer...
media/cast/framer/framer_unittest.cc:38: bool multiple = false;
On 2014/05/16 19:45:25, miu wrote:
> In all these tests, consider testing that "multiple" was set correctly.  ;-)

Done.

https://codereview.chromium.org/289483003/diff/40001/media/cast/video_receive...
File media/cast/video_receiver/video_receiver.cc (right):

https://codereview.chromium.org/289483003/diff/40001/media/cast/video_receive...
media/cast/video_receiver/video_receiver.cc:158: bool
have_multiple_complete_frames = false;
On 2014/05/16 19:45:25, miu wrote:
> Please make these changes identically in AudioReceiver.  I promise I'll merge
> the two shortly.  ;-)

Done.

[miu, 2014-05-19 23:53:59]

lgtm

https://codereview.chromium.org/289483003/diff/60001/media/cast/framer/framer...
File media/cast/framer/framer_unittest.cc (right):

https://codereview.chromium.org/289483003/diff/60001/media/cast/framer/framer...
media/cast/framer/framer_unittest.cc:464: 
Perhaps we should add a test where we expect multiple to be true?  ;-)

[hubbe, 2014-05-20 00:08:20]

https://codereview.chromium.org/289483003/diff/60001/media/cast/framer/framer...
File media/cast/framer/framer_unittest.cc (right):

https://codereview.chromium.org/289483003/diff/60001/media/cast/framer/framer...
media/cast/framer/framer_unittest.cc:464: 
On 2014/05/19 23:53:59, miu wrote:
> Perhaps we should add a test where we expect multiple to be true?  ;-)

Done, sort of. I had forgotten to check these. Some of them have been changed to
EXPECT_TRUE.

[miu, 2014-05-20 00:29:10]

lgtm

[hubbe, 2014-05-21 16:36:11]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:56:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/289483003/100001

[commit-bot: I haz the power, 2014-05-22 01:29:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-22 01:35:04]

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

[commit-bot: I haz the power, 2014-05-22 01:35:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[miu, 2014-05-23 23:20:42]

lgtm

https://codereview.chromium.org/289483003/diff/120001/media/cast/framer/frame...
File media/cast/framer/framer.cc (right):

https://codereview.chromium.org/289483003/diff/120001/media/cast/framer/frame...
media/cast/framer/framer.cc:64: 
nit: extra newline

[miu, 2014-05-23 23:20:47]

The CQ bit was unchecked by miu@chromium.org

[hubbe, 2014-05-23 23:22:04]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-05-23 23:23:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/289483003/140001

[commit-bot: I haz the power, 2014-05-24 12:17:56]

Change committed as 272709