MSE: Allow fully discarding frames with resulting timestamps below 0

MSE: Allow fully discarding frames with resulting timestamps below 0

This change moves the check of PTS & DTS >= time 0 to after any
append window filtering, as one approach for allowing full preroll
buffer discard without requiring apps start play somewhere other
than time 0.

R=acolwell@chromium.org,dalecurtis@chromium.org
BUG=377059
TEST=No regression, new test added

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

[wolenetz, 2014-05-24 00:17:24]

PTAL @ PS1.

acolwell@, especially review this due to divergence from spec. As I noted in the
bug, "the app, of course, could instead seek to a later time and set
timestampOffset and append window accordingly such that these steps in the spec
wouldn't need changing."

wdyt?

[DaleCurtis, 2014-05-24 00:27:03]

lgtm, I defer to acolwell@ though.  Note that this is inline with the current
behavior with the old frame processor.

[acolwell GONE FROM CHROMIUM, 2014-05-24 05:27:36]

https://codereview.chromium.org/294393002/diff/1/media/filters/frame_processo...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/294393002/diff/1/media/filters/frame_processo...
media/filters/frame_processor.cc:295: // filtering to allow more than 1 full
frame preroll to support gapless
nit: I don't think this comment needs to be AAC specific. I think it is
sufficient to just put this in terms of timestampOffset shifting a media segment
in such a way that some of the coded frames happen to be before 0.

https://codereview.chromium.org/294393002/diff/1/media/filters/frame_processo...
media/filters/frame_processor.cc:302: if (presentation_timestamp <
base::TimeDelta() ||
Yes. I believe the spec should probably be updated to move this step here. The
constraints on setting append_window_start essentially enforces
presentation_timestamp >= 0 at this point so you could probably just change the
presentation_timestamp condition to a DCHECK. The only question remains is
whether we should allow negative DTS through here. For now, I think the answer
should be no. 

Matt, I realize that the "sequence" mode with B-frames issue may still be
something we have to deal with, but for now I think the code should still
enforce DTS >= 0 until we have a better understanding of what the potential
issues are.

[wolenetz, 2014-05-27 19:35:14]

acolwell@, PTAL @ PS2. This can wait until after 5/30.

https://codereview.chromium.org/294393002/diff/1/media/filters/frame_processo...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/294393002/diff/1/media/filters/frame_processo...
media/filters/frame_processor.cc:295: // filtering to allow more than 1 full
frame preroll to support gapless
On 2014/05/24 05:27:36, acolwell_OOO_5-24_to_5-30 wrote:
> nit: I don't think this comment needs to be AAC specific. I think it is
> sufficient to just put this in terms of timestampOffset shifting a media
segment
> in such a way that some of the coded frames happen to be before 0.

Done.

https://codereview.chromium.org/294393002/diff/1/media/filters/frame_processo...
media/filters/frame_processor.cc:302: if (presentation_timestamp <
base::TimeDelta() ||
On 2014/05/24 05:27:36, acolwell_OOO_5-24_to_5-30 wrote:
> Yes. I believe the spec should probably be updated to move this step here. The
> constraints on setting append_window_start essentially enforces
> presentation_timestamp >= 0 at this point so you could probably just change
the
> presentation_timestamp condition to a DCHECK. The only question remains is
> whether we should allow negative DTS through here. For now, I think the answer
> should be no. 
> 
> Matt, I realize that the "sequence" mode with B-frames issue may still be
> something we have to deal with, but for now I think the code should still
> enforce DTS >= 0 until we have a better understanding of what the potential
> issues are.

For PTS, added DCHECK.
For DTS < 0: I've added a note and updated the DVLOG. I did not add a TODO or a
bug; we're not sure yet about MP4 sequence mode layout test failure cause
(parser, test media, or spec).

[acolwell GONE FROM CHROMIUM, 2014-06-02 21:36:19]

lgtm

[wolenetz, 2014-06-02 23:05:08]

The new test needed a slight modification after rebasing onto ToT that now
includes Dale's change enabling full-discard of a decoder preroll buffer
incident on appendWindowStart.

Both acolwell@ and dalecurtis@:
Please take another look (PS4).

Thanks!

[DaleCurtis, 2014-06-02 23:11:03]

lgtm

https://codereview.chromium.org/294393002/diff/50001/media/filters/frame_proc...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/294393002/diff/50001/media/filters/frame_proc...
media/filters/frame_processor_unittest.cc:221: ss << "FullDiscard";
Up to you, but you could just use "P" for consistency with the other tests.

https://codereview.chromium.org/294393002/diff/50001/media/filters/frame_proc...
media/filters/frame_processor_unittest.cc:584:
SetTimestampOffset(frame_duration_ * -2);
Hmm, slightly unrelated, but I wonder if we should invalidate preroll on
timestamp offset changes too.

[wolenetz, 2014-06-03 00:00:19]

Thanks Dale.
acolwell@, PTAL @ PS5 since there have been changes since your PS2 lgtm.

https://codereview.chromium.org/294393002/diff/50001/media/filters/frame_proc...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/294393002/diff/50001/media/filters/frame_proc...
media/filters/frame_processor_unittest.cc:221: ss << "FullDiscard";
On 2014/06/02 23:11:04, DaleCurtis wrote:
> Up to you, but you could just use "P" for consistency with the other tests.

Done.

https://codereview.chromium.org/294393002/diff/50001/media/filters/frame_proc...
media/filters/frame_processor_unittest.cc:584:
SetTimestampOffset(frame_duration_ * -2);
On 2014/06/02 23:11:04, DaleCurtis wrote:
> Hmm, slightly unrelated, but I wonder if we should invalidate preroll on
> timestamp offset changes too.

sgtm for a separate CL.

[acolwell GONE FROM CHROMIUM, 2014-06-03 00:34:01]

lgtm

[wolenetz, 2014-06-03 00:34:21]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-06-03 00:36:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wolenetz@chromium.org/294393002/70001

[commit-bot: I haz the power, 2014-06-03 12:03:14]

Change committed as 274473