1207478 - High timestamps in DASH live stream will lead to an overflow causing samples to be dropped

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Example http://people.mozilla.org/~jyavenard/tests/mse_mp4/overflow.html

The cause is the MP4Demuxer conversion to microseconds:
    return aTimescaleUnits * 1000000ll / mTimescale;


if aTimescaleUnits is > INT64_MAX / 1000000ll; it will overflow leading to negative numbers.

The samples will then be dropped.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Use a reduced fraction to convert to microseconds

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Prevent potential division by zero.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8664826 [details] [diff] [review]
P1. Use a reduced fraction to convert to microseconds

Review of attachment 8664826 [details] [diff] [review]:
-----------------------------------------------------------------

GCD(1m,44.1k)=100, mFactor will drop to 10,000, so there could still be an overflow if aTimescaleUnits>INT64_MAX/10000.
You can probably build a test file for that like you've done for the original issue.

Though better than before, it's not perfect, so I hesitate to r+ something that won't actually solve the full issue -- you have taught me well!
And it could be made worse with a timescale that is relatively prime with 1m (e.g., 44099), in which case the GCD of 1 wouldn't help at all.

I'd suggest you add a check for very large numbers (and give up playing?), or find another algorithm that can handle any number that could be present (even if meaningless) in an MP4 file.

::: media/libstagefright/binding/MoofParser.cpp
@@ +647,5 @@
>      reader->DiscardRemaining();
>      return;
>    }
> +  // Calculate GCD(mFactor, mTimescale)
> +  // Using simple Euclide algorithm as we only calculate it once and the range

Euclidean

Comment 4

[Jean-Yves Avenard [:jya]]

We will already ignore those data later.
plus we have no way of reporting the error.

the timescale is typically the sampling rate so you won't see 44099, and for that particular stream (and most I've seen) it's 48000

we have no way of solving that problem without losing accuracy, at worse if we know we're going to overflow, we could use double instead to calculate the first, and find a way to determine the following samples so that the rounding stays constant across all samples.

And in the mean time, we don't play any BBC live streams and 42 has entered beta.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8664826 [details] [diff] [review]
P1. Use a reduced fraction to convert to microseconds

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> We will already ignore those data later.
> plus we have no way of reporting the error.
> 
> the timescale is typically the sampling rate so you won't see 44099, and for
> that particular stream (and most I've seen) it's 48000
> 
> we have no way of solving that problem without losing accuracy, at worse if
> we know we're going to overflow, we could use double instead to calculate
> the first, and find a way to determine the following samples so that the
> rounding stays constant across all samples.
> 
> And in the mean time, we don't play any BBC live streams and 42 has entered
> beta.

Alright then, let's get this partial solution in, but I'd really like to see another bug to remind us of the remaining issue.

Comment 6

[Jean-Yves Avenard [:jya]]

Let's take another approach then, not handling an overflow, but ensure that an overflow only occurs when it *must* occurs

Comment 7

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

Can't we just do |(float)aTimescaleUnits * 1000000ll| to avoid integer overflow?

Comment 8

[Jean-Yves Avenard [:jya]]

float has a mantissa of 24 bits only ; it will cause unacceptable rounding.
in fact even double would lose accuracy for the case we care.

something like:
  Microseconds ToMicroseconds(int64_t aTimescaleUnits)
  {
    int64_t major = aTimescaleUnits / mTimescale;
    int64_t remainder = aTimescaleUnits % mTimescale;
    return major * 1000000ll + remainder * 1000000ll / mTimescale;
  }

will do the trick, and leave us safe until year 1/1/294441 (assuming those streams use unix timestamps)

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Prevent microseconds calculation overflow.

This leaves us safe until year 294441 (assuming unix timestamp).

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8665185 [details] [diff] [review]
Prevent microseconds calculation overflow.

Review of attachment 8665185 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Should we still raise a bug to handle ill-formed MP4s? (Discard or shift them?)

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #10)
> Comment on attachment 8665185 [details] [diff] [review]
> Prevent microseconds calculation overflow.
> 
> Review of attachment 8665185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.
> 
> Should we still raise a bug to handle ill-formed MP4s? (Discard or shift
> them?)

we can, or leave the responsibility to whomever will manage the Firefox 2534350 release.

Even if we shift ; we'll have the problem at the time the overflow would happen as we can't transition from the non-shifted timestamp to shifted. not easily at least.

Comment 12

[Jean-Yves Avenard [:jya]]

we still have an overflow in VideoUtils::UsecsToFrames ; not sure of the point to use the number of frames here.
need to rework the MDSM there

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: P3. Reduce chance of overflow.

This limit the chance of overflow and we will overflow only if the resulting value can't fit in a 64 bits.

The operation is slower, clang and gcc will optimise the first division + modulo into a single idivq + movq (as idivq calculate both the divisor and remainder) we do have an extra call to idivq : so overall the operation is about twice as slow as before. It's not in any critical loop however and we're talking about 3 more instructions.
It could slow down the seek on B2G where the ARM compiler isn't as efficient. However, we can optimise this by not calling UsecToFrames until absolutely necessary. Very likely irrelevant in comparison to the time it takes to decode the frame anyway, which is full of floatting point calculations.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: P4: Make it impossible for the MDSM frame calculation to overflow.

This should make P3 unnecessary as far as the MDSM is concerned.

I notice that it always calculate the duration of the sample from the number of frames it contains. But why is that? We have an GetEndTime() that provide the duration of the frames without any extra calculation.

Comment 15

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

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> I notice that it always calculate the duration of the sample from the number
> of frames it contains. But why is that? We have an GetEndTime() that provide
> the duration of the frames without any extra calculation.

I also wonder that. IIRC, it is because the end time stamp is not very reliable. Maybe Chirs knows better.

Comment 16

[Jean-Yves Avenard [:jya]]

All the reader's I'm familiar with, will set the duration of the AudioData properly and is just as accurate (in fact, most of them have already calculated the duration from the number of decoded samples).

In the mean time, the logic with the patch is the same as before; just only perform the operation to convert to frames on the seekTime - audioTime ; this can never overflow unlike UsecsToFrame(seekTime) - USecsToFrame(audioTime)

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a26e9d557ad4
https://hg.mozilla.org/integration/mozilla-inbound/rev/016d6f4c79bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef8751efbba
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e634cc7b44a

Comment 18

[Jean-Yves Avenard [:jya]]

ni myself to request uplift to beta

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a26e9d557ad4
https://hg.mozilla.org/mozilla-central/rev/016d6f4c79bd
https://hg.mozilla.org/mozilla-central/rev/2ef8751efbba
https://hg.mozilla.org/mozilla-central/rev/8e634cc7b44a

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8665185 [details] [diff] [review]
Prevent microseconds calculation overflow.

Request is for all 4 patches.

Approval Request Comment
[Feature/regressing bug #]: Bug 641718, changed the internal time format from milliseconds to microseconds making the overflow more likely, the problematic algorithm was introduced in bug 576539.
We never played live streams before 42 is the only version where we finally supports MSE live stream playback. Live streams using unix time (number of seconds since 1/1/1970 will cause our conversion to microseconds to overflow). The overflow only occurs due to the way we performed the conversion
[User impact if declined]: Playback will not start and stall and people will use Chrome that works or web site will continue to server flash content. MSE is one of the new feature in 42.
[Describe test coverage new/current, TreeHerder]: In central, local testing. We have lots of media mochitests. Confirmed with 3rd party that it now works.
[Risks and why]: Low. Instead of doing a * b / c (where a*b will overflow), we do (int(a/c))*b + ((a%c)*b). There is an extra calculation overhead however the impact was mentioned in comment 13.
[String/UUID change made/needed]: None.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8664827 [details] [diff] [review]
P2. Prevent potential division by zero.

Polish of a new feature, taking it.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68a24a8ae1e9
https://hg.mozilla.org/releases/mozilla-beta/rev/71e69002ad13
https://hg.mozilla.org/releases/mozilla-beta/rev/e39181eba4a6
https://hg.mozilla.org/releases/mozilla-beta/rev/984f8c8e519a

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ae10b93c0465
https://hg.mozilla.org/releases/mozilla-aurora/rev/bfa5ce9f77b2
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c6a79ea4a1d
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ecc66b1c925