1065715 - Introduce type-safe time units for media code to prevent common bugs

Status: RESOLVED DUPLICATE of bug 1159578

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

Description

[Matthew Gregan [:kinetik]]

We've had a number of bugs in the past, either in release or during development, that would be avoidable with some type safety introduced to the various media time units.

I propose:

- Introducing type safe time units and converting the current code to use them
  (The code under fmp4/ is already using "Microseconds" (a typedef of int64_t),
   so replacing the typedef with a type is probably a good place to start.)

  From memory we need, at least:
  - Seconds (double, used in public API facing areas)
  - Microseconds (int64_t, used in internal media code)
  - Nanoseconds (int64_t, used in WebM internal code, possibly elsewhere)
  - And possibly "Samples"/"Frames" (used in audio code)

The other issue that has caused a number of bugs in the past is mixing timeline calculations between start-time based and zero-based timelines.  That's also worth addressing, but may make sense to do in another bug.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Do we need something like boost.units (http://www.boost.org/doc/libs/1_54_0/doc/html/boost_units/Examples.html#boost_units.Examples.UnitExample) or something less over-kill?

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Matthew Gregan [:kinetik] from comment #0)
>   - And possibly "Samples"/"Frames" (used in audio code)

The MP4 demuxer uses per-track timescale units.

Comment 3

[Paul Adenot (:padenot)]

That would be very cool, yes.

I also seem to recall that WMF and other Windows things use hundredth of nanoseconds [1].

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMFReader.cpp#236

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Paul Adenot (:padenot) (on PTO until the 13th of October) from comment #3)
> I also seem to recall that WMF and other Windows things use hundredth of
> nanoseconds [1].

Yeah, WMF and DirectShow. We don't need to do much manipulation of time units in the WMF/DirectShow code, pretty much just a single conversion to usecs. So I think it's OK to omit this use case.

I'd expect the MP4 demuxer to be similar; just convert to our content/media time units in the bindings.

(In reply to Matthew Gregan [:kinetik] from comment #0)
> The other issue that has caused a number of bugs in the past is mixing
> timeline calculations between start-time based and zero-based timelines. 
> That's also worth addressing, but may make sense to do in another bug.

This feels like where the more subtle bugs were to me. Doing it in another bug makes sense to me.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Does it make sense to use the same time units as the MSG?

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Chris Pearce (:cpearce) from comment #5)
> Does it make sense to use the same time units as the MSG?

The MSG time units now depend on the default output audio sample rate.  It may be preferable to have units of fixed length.

Comment 7

[Matthew Gregan [:kinetik]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> This feels like where the more subtle bugs were to me. Doing it in another
> bug makes sense to me.

Bug 1067754 for that.

Comment 8

[Jean-Yves Avenard [:jya]]

Great minds meet I guess... This was done with the media::TimeUnit object ; and we went through exactly the same type of questions as above