1817997 - Rewrite TimeUnit.h

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Paul Adenot (:padenot)]

TimeUnit.h internally uses integer microseconds, which is not precise enough is a number of scenarios, especially for audio. Rounding is inconsistent, and causes subtle issues.

It needs to use a fractional representation, with a numerator and a denominator, (this is often how it works in media systems).

For audio, the denominator frequently is the sample-rate of the audio stream, and the numerator frequently is the frame number (but can be anything as long as it converts to a number of seconds in the end).

For video, it's frequent that the container contains a denominator (called the timestamp scale or time base, or something else, depending on the container) that's related to the frame rate. Again, the numerator allows converting properly to seconds (and any other useful unit).

It's possible to use timebase-based TimeUnit together regardless of the time unit because it all translates back to seconds in the end if needed (with the maximum precision possible, since it's the original number without conversion).

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Rewrite TimeUnit.h r?alwu,#media-playback-reviewers

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Adjust code to take advantage of the new TimeUnit API. r?alwu,#media-playback-reviewers

Depends on D171255

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Add logging in WebMDemuxer::Init failure paths. r?alwu

Comment 5

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update AudioData to use the new TimeUnit facilities, removing rounding issues and making it sample-accurate. r?alwu

Depends on D171256

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Fix logging in AudioTrimmer. r?alwu

Depends on D173304

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update ContainerParser to work with the new TimeUnit. r?alwu,kinetik

Depends on D173305

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update the MP4 demuxer to use TimeUnit based on the internal MP4 time base. r?alwu,kinetik

This still gets the initial time value from mp4parse-rust, that is in
microseconds. mp4parse-rust has been updated to expose real time, and will be
updated later.

Depends on D173306

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update ChannelMediaDecoder to use the new TimeUnit. r?alwu,kinetik

Fairly straightforward.

Depends on D173307

Comment 10

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update MediaDecoder to use the new TimeUnit. r?alwu,kinetik

Just using TimeUnit instead of seconds in doubles. Some IsValid() checks added
because quite NaNs were used previously.

Depends on D173308

Comment 11

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update MediaSourceDecoder to use the new TimeUnit. r?alwu,kinetik

Nothing major, except Infinity is now represented explicitly, and that it's
necessary to lower the precision of some values (using the method
ToMicrosecondResolution()) that end up visible by script to ensure that
there's no change in behaviour. When not doing that, the reported numbers have
higher precision -- we keep with the same precision for now.

Depends on D173309

Comment 12

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update the SourceBuffer and MediaSource implementation to use the new TimeUnit. r?alwu,kinetik

Similarly to the previous patch, it's necessary to reduce the resolution of
values visible from script to keep the exact same behaviour as before. We might
be able to remove this eventually but it might not be very useful to do so, the
benefits of using rational-based time are mostly for internal computations.

Depends on D173310

Comment 13

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update TrackBufferManager to use the new TimeUnit. r?alwu,kinetik

This one tricky bit is when appending frames to the internal buffer. We want to
do all computations in the time base of the container, that's more precise. When
the offset is set by js (as a 64-bits double, converted to integer
microseconds), we convert it to the time base of the track, and accumulate this
way, to have minimal error.

Depends on D173311

Comment 14

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update HTMLMediaElement to use the new TimeUnit. r?alwu,kinetik

Similarly to other commits, it's necessary to sometimes reduce the precision of
returned time values, to ensure there's no change to returned numbers.

Depends on D173312

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Reduce precision when storing time related to the played attribute. r?alwu,kinetik

Depends on D173315

Comment 16

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - When checking if a media has been seeked to the end, take into account the resolution of time values. r?alwu,kinetik

Depends on D173528

Comment 17

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Fix some test. r?alwu

Two tests are adjusted to round differently, this doesn't have real
consequences.

For the third test the result should really be 0 (the segments are of the same
duration), but it was a very small number (about 1us in seconds) before because
of rounding.

Depends on D173529

Comment 18

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Replace more double representing seconds by TimeUnit in MediaSource. r?alwu,kinetik

Depends on D176036

Comment 19

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update logging in AudioTrimmer to show ticks and base. r?alwu,kinetik

Depends on D176037

Comment 20

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Remove FramesToTimeUnit, replacing it by the TimeUnit constructor. r?alwu,kinetik

Depends on D176038

Comment 21

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Fix warnings around TimeUnit in various files. r?alwu

Depends on D176039

Comment 22

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Limit time resolution to microsecond on values exposed to script. r?alwu

Depends on D176040

Comment 23

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Write a version of GetSeekableTimeRanges that uses double and not TimeUnit. r?alwu

When playing a MediaSource, seekable should return [0, duration], and duration
can be a double of any precision.

Depends on D176041

Comment 24

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Fix test TestvideoTrackEncoder.cpp failure cause by the new TimeUnit. r?pehrsons

The new unit simply accumulates the internal mp4 ticks, no rounding occur.

Depends on D176042

Comment 25

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Properly print invalid TimeUnit. r?alwu

Depends on D176043

Comment 26

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Allow creating a zero TimeUnit of a particular base. r?alwu

Depends on D176044

Comment 27

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update mp4parse to 888ce901bb604d9e385. r?kinetik

Depends on D176045

Comment 28

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update nsAVIFDecoder to use the new mp4parse-rust time units. r?kinetik

Depends on D176047

Comment 29

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Update mp4parse-rust to revision cf8b0e04. r?#media-playback-reviewers

Depends on D176049

Comment 30

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Handle overflowing TimeUnit more gracefully. r?alwu

Some of those are not avoidable: we use int64_t inside TimeUnit, and mp4 uses
u64 as the numerator. It's not problematic in practice.

Depends on D176050

Comment 31

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Fix mp4_demuxer/TestParser.cpp after the mp4 parser changes. r?alwu

This switches to using seconds, adds more decimals, and uses proper floating
point comparison functions.

We're not using microseconds here because this test intentionally uses value
that are the maximum and minimum of int64_t and that overflows TimeUnit.

Depends on D176051

Comment 32

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Allow getting a TimeUnit from a time expressed in hundreds of nanoseconds. r?alwu

This is useful when dealing with Windows media APIs.

Depends on D176052

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b20cc750c020
Add logging in WebMDemuxer::Init failure paths. r=alwu
https://hg.mozilla.org/integration/autoland/rev/41c0bd1133c5
Rewrite TimeUnit.h r=alwu
https://hg.mozilla.org/integration/autoland/rev/697ec1928f9c
Adjust code to take advantage of the new TimeUnit API. r=alwu
https://hg.mozilla.org/integration/autoland/rev/2d88a5915db9
Update AudioData to use the new TimeUnit facilities, removing rounding issues and making it sample-accurate. r=alwu
https://hg.mozilla.org/integration/autoland/rev/ea1aebd4e245
Fix logging in AudioTrimmer. r=alwu
https://hg.mozilla.org/integration/autoland/rev/66e4a26e1ebe
Update ContainerParser to work with the new TimeUnit. r=alwu,kinetik
https://hg.mozilla.org/integration/autoland/rev/89c241451a32
Update the MP4 demuxer to use TimeUnit based on the internal MP4 time base. r=alwu
https://hg.mozilla.org/integration/autoland/rev/7f64e65d9497
Update ChannelMediaDecoder to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/29f7cd2b341c
Update MediaDecoder to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/7af506896930
Update MediaSourceDecoder to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/8e0dcf163138
Update the SourceBuffer and MediaSource implementation to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/412e0a9ec4e8
Update TrackBufferManager to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/b86b569ecd46
Update HTMLMediaElement to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/c1ac4c31ad9a
Reduce precision when storing time related to the played attribute. r=alwu
https://hg.mozilla.org/integration/autoland/rev/2952a6fa06e5
When checking if a media has been seeked to the end, take into account the resolution of time values. r=alwu
https://hg.mozilla.org/integration/autoland/rev/d096b9cc905d
Fix some test. r=alwu
https://hg.mozilla.org/integration/autoland/rev/e589b4d30995
Replace more double representing seconds by TimeUnit in MediaSource. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/f516a428ba32
Update logging in AudioTrimmer to show ticks and base. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/35ab3ed5e3fc
Remove FramesToTimeUnit, replacing it by the TimeUnit constructor. r=alwu,kinetik
https://hg.mozilla.org/integration/autoland/rev/25ecea0f3d49
Fix warnings around TimeUnit in various files. r=alwu
https://hg.mozilla.org/integration/autoland/rev/deea3077f762
Limit time resolution to microsecond on values exposed to script. r=alwu
https://hg.mozilla.org/integration/autoland/rev/670c599a3b99
Write a version of GetSeekableTimeRanges that uses double and not TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/8fca27bd0f6b
Fix test TestvideoTrackEncoder.cpp failure cause by the new TimeUnit. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/d5d5d390dcbf
Properly print invalid TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/b9f2e1155bd7
Allow creating a zero TimeUnit of a particular base. r=alwu
https://hg.mozilla.org/integration/autoland/rev/54c3294f0839
Update mp4parse to 888ce901bb604d9e385. r=kinetik,glandium
https://hg.mozilla.org/integration/autoland/rev/14b061d0f595
Update nsAVIFDecoder to use the new mp4parse-rust time units. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/0ef13d357ee3
Update mp4parse-rust to revision cf8b0e04. r=media-playback-reviewers,glandium,kinetik
https://hg.mozilla.org/integration/autoland/rev/2d92f903dd66
Handle overflowing TimeUnit more gracefully. r=alwu
https://hg.mozilla.org/integration/autoland/rev/4b42659090f3
Fix mp4_demuxer/TestParser.cpp after the mp4 parser changes. r=alwu
https://hg.mozilla.org/integration/autoland/rev/011cece33b0d
Allow getting a TimeUnit from a time expressed in hundreds of nanoseconds. r=alwu

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2113c9b661f
Fix nightly-as-release build.

Comment 35

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/b20cc750c020
https://hg.mozilla.org/mozilla-central/rev/41c0bd1133c5
https://hg.mozilla.org/mozilla-central/rev/697ec1928f9c
https://hg.mozilla.org/mozilla-central/rev/2d88a5915db9
https://hg.mozilla.org/mozilla-central/rev/ea1aebd4e245
https://hg.mozilla.org/mozilla-central/rev/66e4a26e1ebe
https://hg.mozilla.org/mozilla-central/rev/89c241451a32
https://hg.mozilla.org/mozilla-central/rev/7f64e65d9497
https://hg.mozilla.org/mozilla-central/rev/29f7cd2b341c
https://hg.mozilla.org/mozilla-central/rev/7af506896930
https://hg.mozilla.org/mozilla-central/rev/8e0dcf163138
https://hg.mozilla.org/mozilla-central/rev/412e0a9ec4e8
https://hg.mozilla.org/mozilla-central/rev/b86b569ecd46
https://hg.mozilla.org/mozilla-central/rev/c1ac4c31ad9a
https://hg.mozilla.org/mozilla-central/rev/2952a6fa06e5
https://hg.mozilla.org/mozilla-central/rev/d096b9cc905d
https://hg.mozilla.org/mozilla-central/rev/e589b4d30995
https://hg.mozilla.org/mozilla-central/rev/f516a428ba32
https://hg.mozilla.org/mozilla-central/rev/35ab3ed5e3fc
https://hg.mozilla.org/mozilla-central/rev/25ecea0f3d49
https://hg.mozilla.org/mozilla-central/rev/deea3077f762
https://hg.mozilla.org/mozilla-central/rev/670c599a3b99
https://hg.mozilla.org/mozilla-central/rev/8fca27bd0f6b
https://hg.mozilla.org/mozilla-central/rev/d5d5d390dcbf
https://hg.mozilla.org/mozilla-central/rev/b9f2e1155bd7
https://hg.mozilla.org/mozilla-central/rev/54c3294f0839
https://hg.mozilla.org/mozilla-central/rev/14b061d0f595
https://hg.mozilla.org/mozilla-central/rev/0ef13d357ee3
https://hg.mozilla.org/mozilla-central/rev/2d92f903dd66
https://hg.mozilla.org/mozilla-central/rev/4b42659090f3
https://hg.mozilla.org/mozilla-central/rev/011cece33b0d
https://hg.mozilla.org/mozilla-central/rev/f2113c9b661f

Comment 36

[Cosmin Sabou [:CosminS]]

Backed out 80 changesets (bug 1821362, bug 1703812, bug 1817997) for causing media crashes as in Bug 1833890.

Backout link: https://hg.mozilla.org/mozilla-central/rev/225c5ab0d999e743db5298d125893ae0702884af

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2783a65c89d2
Add logging in WebMDemuxer::Init failure paths. r=alwu
https://hg.mozilla.org/integration/autoland/rev/ca9a94f33590
Rewrite TimeUnit.h r=alwu
https://hg.mozilla.org/integration/autoland/rev/1346f08e6f53
Adjust code to take advantage of the new TimeUnit API. r=alwu
https://hg.mozilla.org/integration/autoland/rev/78068c6cc38f
Update AudioData to use the new TimeUnit facilities, removing rounding issues and making it sample-accurate. r=alwu
https://hg.mozilla.org/integration/autoland/rev/9390bcf9c450
Fix logging in AudioTrimmer. r=alwu
https://hg.mozilla.org/integration/autoland/rev/5c1b6ce71ca1
Update ContainerParser to work with the new TimeUnit. r=alwu,kinetik
https://hg.mozilla.org/integration/autoland/rev/aa102eb9990d
Update the MP4 demuxer to use TimeUnit based on the internal MP4 time base. r=alwu
https://hg.mozilla.org/integration/autoland/rev/ab33bd048f67
Update ChannelMediaDecoder to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/c134f0bfd5a7
Update MediaDecoder to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/a724c0f009c6
Update MediaSourceDecoder to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/e62d04495fb9
Update the SourceBuffer and MediaSource implementation to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/4b27b9b20884
Update TrackBufferManager to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/280227568779
Update HTMLMediaElement to use the new TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/9d931c8093da
Reduce precision when storing time related to the played attribute. r=alwu
https://hg.mozilla.org/integration/autoland/rev/d4b97015415c
When checking if a media has been seeked to the end, take into account the resolution of time values. r=alwu
https://hg.mozilla.org/integration/autoland/rev/286d9df49c11
Fix some test. r=alwu
https://hg.mozilla.org/integration/autoland/rev/ab6f987c4ffb
Replace more double representing seconds by TimeUnit in MediaSource. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/b3bb60a62ab2
Update logging in AudioTrimmer to show ticks and base. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/881e88b4838f
Remove FramesToTimeUnit, replacing it by the TimeUnit constructor. r=alwu,kinetik
https://hg.mozilla.org/integration/autoland/rev/3ed87ad54085
Fix warnings around TimeUnit in various files. r=alwu
https://hg.mozilla.org/integration/autoland/rev/f4449389bc7e
Limit time resolution to microsecond on values exposed to script. r=alwu
https://hg.mozilla.org/integration/autoland/rev/2a6dd728562a
Write a version of GetSeekableTimeRanges that uses double and not TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/6b1e4a1707b4
Fix test TestvideoTrackEncoder.cpp failure cause by the new TimeUnit. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/dfbcc10a5e18
Properly print invalid TimeUnit. r=alwu
https://hg.mozilla.org/integration/autoland/rev/5a913935f40c
Allow creating a zero TimeUnit of a particular base. r=alwu
https://hg.mozilla.org/integration/autoland/rev/d60b7a400022
Update mp4parse to 888ce901bb604d9e385. r=kinetik,glandium
https://hg.mozilla.org/integration/autoland/rev/989a3c429c92
Update nsAVIFDecoder to use the new mp4parse-rust time units. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/f6514dccc516
Update mp4parse-rust to revision cf8b0e04. r=media-playback-reviewers,glandium,kinetik
https://hg.mozilla.org/integration/autoland/rev/28f2d9948ecf
Handle overflowing TimeUnit more gracefully. r=alwu
https://hg.mozilla.org/integration/autoland/rev/dc3d93ab3284
Fix mp4_demuxer/TestParser.cpp after the mp4 parser changes. r=alwu
https://hg.mozilla.org/integration/autoland/rev/2ea75ac1b073
Allow getting a TimeUnit from a time expressed in hundreds of nanoseconds. r=alwu

Comment 38

[Paul Adenot (:padenot)]

Attachment: Bug 1817997 - Inline a computation used in a MOZ_DIAGNOSTIC_ASSERT to fix the build on non-nightly builds.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56ed4f603735
Inline a computation used in a MOZ_DIAGNOSTIC_ASSERT to fix the build on non-nightly builds. CLOSED TREE

Comment 40

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2783a65c89d2
https://hg.mozilla.org/mozilla-central/rev/ca9a94f33590
https://hg.mozilla.org/mozilla-central/rev/1346f08e6f53
https://hg.mozilla.org/mozilla-central/rev/78068c6cc38f
https://hg.mozilla.org/mozilla-central/rev/9390bcf9c450
https://hg.mozilla.org/mozilla-central/rev/5c1b6ce71ca1
https://hg.mozilla.org/mozilla-central/rev/aa102eb9990d
https://hg.mozilla.org/mozilla-central/rev/ab33bd048f67
https://hg.mozilla.org/mozilla-central/rev/c134f0bfd5a7
https://hg.mozilla.org/mozilla-central/rev/a724c0f009c6
https://hg.mozilla.org/mozilla-central/rev/e62d04495fb9
https://hg.mozilla.org/mozilla-central/rev/4b27b9b20884
https://hg.mozilla.org/mozilla-central/rev/280227568779
https://hg.mozilla.org/mozilla-central/rev/9d931c8093da
https://hg.mozilla.org/mozilla-central/rev/d4b97015415c
https://hg.mozilla.org/mozilla-central/rev/286d9df49c11
https://hg.mozilla.org/mozilla-central/rev/ab6f987c4ffb
https://hg.mozilla.org/mozilla-central/rev/b3bb60a62ab2
https://hg.mozilla.org/mozilla-central/rev/881e88b4838f
https://hg.mozilla.org/mozilla-central/rev/3ed87ad54085
https://hg.mozilla.org/mozilla-central/rev/f4449389bc7e
https://hg.mozilla.org/mozilla-central/rev/2a6dd728562a
https://hg.mozilla.org/mozilla-central/rev/6b1e4a1707b4
https://hg.mozilla.org/mozilla-central/rev/dfbcc10a5e18
https://hg.mozilla.org/mozilla-central/rev/5a913935f40c
https://hg.mozilla.org/mozilla-central/rev/d60b7a400022
https://hg.mozilla.org/mozilla-central/rev/989a3c429c92
https://hg.mozilla.org/mozilla-central/rev/f6514dccc516
https://hg.mozilla.org/mozilla-central/rev/28f2d9948ecf
https://hg.mozilla.org/mozilla-central/rev/dc3d93ab3284
https://hg.mozilla.org/mozilla-central/rev/2ea75ac1b073
https://hg.mozilla.org/mozilla-central/rev/56ed4f603735