Update duration when demuxed packets exceed known duration.

Update duration when demuxed packets exceed known duration.

This applies a similar algorithm to what MSE does, if we see a packet
greater than the current duration or the duration is unknown, notify
Blink of the updated duration.

This prevents us from ending playback earlier on clips which report
incorrect durations; specifically those which report 0.1s or similar.

The UX experience is not great when this occurs, but is similar to what
happens in other browsers. The timeline will increasingly fall behind
as more content is demuxed and seeking is hard to accomplish.

BUG=645998, 656599
TEST=updated test expectations.

Review-Url: https://codereview.chromium.org/2645953004
Cr-Commit-Position: refs/heads/master@{#446889}
Committed: https://chromium.googlesource.com/chromium/src/+/560640b659f688387d4c239e00f2dc941607e9f0

[DaleCurtis, 2017-01-20 20:43:48]

dalecurtis@chromium.org changed reviewers:
+ chcunningham@chromium.org

[DaleCurtis, 2017-01-20 20:43:50]

This passed media_unittests, still need to see what happens to layout tests. So
dry running right now.

[DaleCurtis, 2017-01-20 20:43:54]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 20:44:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-01-20 22:11:56]

Le sigh, some layout tests will need some updating, failures:
media/video-currentTime-set.html
media/video-seek-past-end-paused.html
media/video-played-collapse.html
media/video-seek-to-duration-with-playbackrate-zero.html
media/event-attributes.html

[commit-bot: I haz the power, 2017-01-20 22:23:56]

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

[commit-bot: I haz the power, 2017-01-20 22:23:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[DaleCurtis, 2017-01-20 23:07:46]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 23:08:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-01-21 00:00:55]

The CQ bit was unchecked by dalecurtis@chromium.org

[chcunningham, 2017-01-23 19:14:10]

https://codereview.chromium.org/2645953004/diff/20001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2645953004/diff/20001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:569: const base::TimeDelta new_duration =
last_packet_timestamp_;
Should this be last_packet_timestamp_ + last_packet_duration_?

https://codereview.chromium.org/2645953004/diff/20001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:1721: duration_known_ ? stream->duration() :
stream->GetElapsedTime();
It seems like now that stream->duration() is automatically updating, we should
be able to get rid of stream->GetElapsedTime(). IIUC, GetElapsedTime is only
being used when reading the first frame. Could we just make duration() do the
behavior of GetElapsedTime() whenever FFmpegDemxuerStream::duration_ is
kNoTimestamp?

[DaleCurtis, 2017-01-24 02:05:19]

https://codereview.chromium.org/2645953004/diff/20001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2645953004/diff/20001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:569: const base::TimeDelta new_duration =
last_packet_timestamp_;
On 2017/01/23 at 19:14:10, chcunningham wrote:
> Should this be last_packet_timestamp_ + last_packet_duration_?

Adding duration explodes a lot of stuff in our tests and on all the real world
content in my vault; it appears to be frequently inaccurate for the last packet
in the stream. E.g., bear-320x240.webm will increase in duration, due to the
audio stream, from 2744ms to 2767ms, but the actual audio stream is truly 2744ms
(verified by inspection of wave data). As such I think we should leave it as is.

https://codereview.chromium.org/2645953004/diff/20001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:1721: duration_known_ ? stream->duration() :
stream->GetElapsedTime();
On 2017/01/23 at 19:14:10, chcunningham wrote:
> It seems like now that stream->duration() is automatically updating, we should
be able to get rid of stream->GetElapsedTime(). IIUC, GetElapsedTime is only
being used when reading the first frame. Could we just make duration() do the
behavior of GetElapsedTime() whenever FFmpegDemxuerStream::duration_ is
kNoTimestamp?

This fails a couple tests, so I'll take a closer look. Seems reasonable though.

https://codereview.chromium.org/2645953004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/video-play-stall-expected.txt
(right):

https://codereview.chromium.org/2645953004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/video-play-stall-expected.txt:1:
This is a testharness.js-based test.
This is incorrect, https://bugs.chromium.org/p/chromium/issues/detail?id=684174
looks like the new clip might have exposed a Multibuffer bug. Will ping for
review once that's resolved.

[DaleCurtis, 2017-01-26 22:38:25]

Patchset #5 (id:80001) has been deleted

[DaleCurtis, 2017-01-26 22:39:51]

This is all ready to go now. Turned out the network issue was due to the crappy
CGI server... which we've had a php replacement for for years! So I just deleted
the CGI server and updated associated tests to use the php version. PTAL.

[DaleCurtis, 2017-01-26 22:57:27]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 22:58:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 01:05:03]

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

[commit-bot: I haz the power, 2017-01-27 01:05:07]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2017-01-27 21:41:13]

Confused by some of the pngs.... we have some that were blank before and now
aren't and visa versa. Do you know whats caused that?

https://codereview.chromium.org/2645953004/diff/100001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/2645953004/diff/100001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer_unittest.cc:850: EXPECT_CALL(host_,
SetDuration(base::TimeDelta::FromMilliseconds(991)));
I'm confused about why the durations came down here. We init stream duration in
the same way as usual, only now we allow it to grow past that starting point.
When we pick up the max duration from the various streams, we now use the
dynamic stream duration() instead of GetElapsedTime(), but I expect those would
be roughly equivalent (or even that duration() would be bigger, since elapsed
time used cur_dts while duration_ uses pts).

With that said, I could imagine the durations going *up*, but coming down is
surprising.

[DaleCurtis, 2017-01-27 21:50:56]

The pixel based tests can vary a lot per platform depending on the scaling
algorithm I think; sometimes we get images which are the same across all
platforms and other times there's some platform specific differences.

I just followed the directions for updating baselines on the webkit
documentation. They all look right as far as I can tell though.

https://codereview.chromium.org/2645953004/diff/100001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/2645953004/diff/100001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer_unittest.cc:850: EXPECT_CALL(host_,
SetDuration(base::TimeDelta::FromMilliseconds(991)));
On 2017/01/27 at 21:41:13, chcunningham wrote:
> I'm confused about why the durations came down here. We init stream duration
in the same way as usual, only now we allow it to grow past that starting point.
When we pick up the max duration from the various streams, we now use the
dynamic stream duration() instead of GetElapsedTime(), but I expect those would
be roughly equivalent (or even that duration() would be bigger, since elapsed
time used cur_dts while duration_ uses pts).
> 
> With that said, I could imagine the durations going *up*, but coming down is
surprising.

I'm not sure, I spot checked a couple of the files (decode to wav, analyze
sample count) and the lengths are accurate now, whereas before they were
slightly too long.

[chcunningham, 2017-01-27 22:58:34]

LGTM

On 2017/01/27 21:50:56, DaleCurtis wrote:
> The pixel based tests can vary a lot per platform depending on the scaling
> algorithm I think; sometimes we get images which are the same across all
> platforms and other times there's some platform specific differences.
> 
> I just followed the directions for updating baselines on the webkit
> documentation. They all look right as far as I can tell though.

I realize now that the ones I was worried about follow a pattern of deleting the
cross-platform-image and replacing it with per-platform-images. Makes sense.

[DaleCurtis, 2017-01-28 00:33:32]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2017-01-28 00:34:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 02:50:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485563612787370,
"parent_rev": "a92d829bc8a9e2c3343010d3691b9da93b7275eb", "commit_rev":
"560640b659f688387d4c239e00f2dc941607e9f0"}

[commit-bot: I haz the power, 2017-01-28 02:50:48]

Description was changed from

==========
Update duration when demuxed packets exceed known duration.

This applies a similar algorithm to what MSE does, if we see a packet
greater than the current duration or the duration is unknown, notify
Blink of the updated duration.

This prevents us from ending playback earlier on clips which report
incorrect durations; specifically those which report 0.1s or similar.

The UX experience is not great when this occurs, but is similar to what
happens in other browsers. The timeline will increasingly fall behind
as more content is demuxed and seeking is hard to accomplish.

BUG=645998, 656599
TEST=updated test expectations.
==========

to

==========
Update duration when demuxed packets exceed known duration.

This applies a similar algorithm to what MSE does, if we see a packet
greater than the current duration or the duration is unknown, notify
Blink of the updated duration.

This prevents us from ending playback earlier on clips which report
incorrect durations; specifically those which report 0.1s or similar.

The UX experience is not great when this occurs, but is similar to what
happens in other browsers. The timeline will increasingly fall behind
as more content is demuxed and seeking is hard to accomplish.

BUG=645998, 656599
TEST=updated test expectations.

Review-Url: https://codereview.chromium.org/2645953004
Cr-Commit-Position: refs/heads/master@{#446889}
Committed:
https://chromium.googlesource.com/chromium/src/+/560640b659f688387d4c239e00f2...
==========

[commit-bot: I haz the power, 2017-01-28 02:50:49]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/560640b659f688387d4c239e00f2...