Switch to using an explicit ended signal instead of time comparison.

Switch to using an explicit ended signal instead of time comparison.

Time comparison is fragile in a world of poorly muxed media, so
instead rely on an explicit ended signal. This also includes a fix
so that the duration is updated correctly in circumstances of poorly
muxed media.

BUG=409280, 645998
TEST=video plays; duration increases.

[DaleCurtis, 2016-10-20 21:09:37]

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

[commit-bot: I haz the power, 2016-10-20 21:10:24]

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

[DaleCurtis, 2016-10-20 21:43:47]

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

[commit-bot: I haz the power, 2016-10-20 21:44:19]

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

[DaleCurtis, 2016-10-20 21:44:36]

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

[DaleCurtis, 2016-10-20 21:44:36]

WDYT? The spec talks about time comparisons, but that's kludgy (floating point
math, bad muxing, etc) so lets use an explicit signal instead!

[commit-bot: I haz the power, 2016-10-20 22:06:50]

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

[commit-bot: I haz the power, 2016-10-20 22:06:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2016-10-20 22:18:10]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2016-10-20 22:18:15]

Patchset #1 (id:20001) has been deleted

[DaleCurtis, 2016-10-20 22:18:23]

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

[commit-bot: I haz the power, 2016-10-20 22:19: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, 2016-10-20 23:22:30]

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

[commit-bot: I haz the power, 2016-10-20 23:22:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mlamouri (slow - plz ping), 2016-10-21 10:43:38]

I do not have that much background here but instead of increasing the duration
while reading frames and discovering that the duration has changed, would it
make sense to switch to an unkwnon duration state? It might help with the
controls UI because it would be indeterminate instead of always moving target
and we could add a ended() signal to HTMLMediaElement instead of
timeChanged(ended) because timeChanged() doesn't call ended if the duration
isn't known.

I'm pretty sure there is a good reason why you didn't do that but I would like
to know why :)

[DaleCurtis, 2016-10-21 17:17:05]

Unknown duration has implications for seekability IIRC, which is why I didn't
take that approach. I'm also less confident in transitions from known to unknown
duration.

[DaleCurtis, 2016-10-21 22:59:18]

On 2016/10/21 at 17:17:05, DaleCurtis wrote:
> Unknown duration has implications for seekability IIRC, which is why I didn't
take that approach. I'm also less confident in transitions from known to unknown
duration.

Things seem to work if we use unknown duration, but I think the UI is a bit
worse. Instead of even allowing a scrubber bar, it's fixed at the zero position
with no seekable range until after the first playthrough. If we update the
duration it's more usable in the sense that the user can seek back and forward.

[DaleCurtis, 2016-10-21 23:34:31]

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

[commit-bot: I haz the power, 2016-10-21 23:35:32]

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

[DaleCurtis, 2016-10-21 23:39:39]

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

[DaleCurtis, 2016-10-21 23:39:40]

+chcunningham since this has a fair bit of media/ code and he was interested in
this :)

[commit-bot: I haz the power, 2016-10-22 00:38:20]

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

[commit-bot: I haz the power, 2016-10-22 00:38:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2016-10-24 10:42:59]

Thanks for looking into the alternative. lgtm

[chcunningham, 2016-10-24 20:18:54]

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

https://codereview.chromium.org/2440563004/diff/60001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:1688: duration_known_ ? stream->duration() :
stream->GetElapsedTime();
why not always pick GetElapsedTime here? Or why not always use
stream->duration()? Should we instead just take the max of these?

https://codereview.chromium.org/2440563004/diff/60001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:1693: if (duration_ == kInfiniteDuration ||
max_duration > duration_) {
I'm confused about when/why we want to override Inifinity. This logic would
allow duration to go from Infinity -> some finite number. But, when  updating
the duration below (:1729), we guard against this, only updating duration if it
was already "known" (not infinite).

[chcunningham, 2016-10-24 20:28:02]

Also, take a look at this bug: 
https://bugs.chromium.org/p/chromium/issues/detail?id=592170

I think this change completely addresses that

[DaleCurtis, 2016-10-24 22:23:59]

Description was changed from

==========
Switch to using an explicit ended signal instead of time comparison.

Time comparison is fragile in a world of poorly muxed media, so
instead rely on an explicit ended signal. This also includes a fix
so that the duration is updated correctly in circumstances of poorly
muxed media.

BUG=645998
TEST=video plays; duration increases.
==========

to

==========
Switch to using an explicit ended signal instead of time comparison.

Time comparison is fragile in a world of poorly muxed media, so
instead rely on an explicit ended signal. This also includes a fix
so that the duration is updated correctly in circumstances of poorly
muxed media.

BUG=409280, 645998
TEST=video plays; duration increases.
==========

[chcunningham, 2016-10-24 22:48:41]

https://codereview.chromium.org/2440563004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2440563004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3194: return dur > 0 &&
now >= dur &&
currentTime is still being compared to duration here. would it be more correct
to use ended_ = true/false, saved from last call to timeChanged (or perhaps made
public accesssor from WMPI interface)?

[DaleCurtis, 2016-10-24 23:29:44]

It's also further complicated by all these failing bots... which are due to
inaccurate duration information in the muxed files and ffmpeg bugs.

E.g., the test.ogv used by many layout tests has a audio stream which continues
past the end of the video track, but metadata duration only includes the video
track.

E.g., the test.webm file sums to a duration of 1.0009999 while the written
duration is 1.001 :/

https://codereview.chromium.org/2440563004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2440563004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3194: return dur > 0 &&
now >= dur &&
On 2016/10/24 at 22:48:41, chcunningham wrote:
> currentTime is still being compared to duration here. would it be more correct
to use ended_ = true/false, saved from last call to timeChanged (or perhaps made
public accesssor from WMPI interface)?

Ah, good find. This is becoming problematic since it'll now start causing less
surgical deviations from the HTML5 video algorithm as proposed by the spec...