Closed Bug 1207478 Opened 4 years ago Closed 4 years ago

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

Categories

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

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

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.
Summary: High timestamps in DASH live stream will lead to an overflow → High timestamps in DASH live stream will lead to an overflow causing samples to be dropped
Attachment #8664826 - Flags: review?(gsquelart)
Attachment #8664827 - Flags: review?(gsquelart)
Assignee: nobody → jyavenard
Attachment #8664827 - Flags: review?(gsquelart) → review+
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
Attachment #8664826 - Flags: review?(gsquelart)
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 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.
Attachment #8664826 - Flags: review+
Attachment #8664826 - Attachment is obsolete: true
Let's take another approach then, not handling an overflow, but ensure that an overflow only occurs when it *must* occurs
Can't we just do |(float)aTimescaleUnits * 1000000ll| to avoid integer overflow?
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)
This leaves us safe until year 294441 (assuming unix timestamp).
Attachment #8665185 - Flags: review?(gsquelart)
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?)
Attachment #8665185 - Flags: review?(gsquelart) → review+
(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.
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
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.
Attachment #8665200 - Flags: review?(jwwang)
Attachment #8665200 - Flags: feedback?(gsquelart)
Attachment #8665200 - Flags: feedback?(gsquelart) → feedback+
Attachment #8665200 - Flags: review?(jwwang) → review+
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.
Attachment #8665269 - Flags: review?(jwwang)
(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.
Flags: needinfo?(cpearce)
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)
Attachment #8665269 - Flags: review?(jwwang) → review+
ni myself to request uplift to beta
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
Attachment #8665185 - Flags: approval-mozilla-beta?
Attachment #8665185 - Flags: approval-mozilla-aurora?
Comment on attachment 8664827 [details] [diff] [review]
P2. Prevent potential division by zero.

Polish of a new feature, taking it.
Attachment #8664827 - Flags: approval-mozilla-beta+
Attachment #8664827 - Flags: approval-mozilla-aurora+
Attachment #8665185 - Flags: approval-mozilla-beta?
Attachment #8665185 - Flags: approval-mozilla-beta+
Attachment #8665185 - Flags: approval-mozilla-aurora?
Attachment #8665185 - Flags: approval-mozilla-aurora+
Attachment #8665200 - Flags: approval-mozilla-beta+
Attachment #8665200 - Flags: approval-mozilla-aurora+
Attachment #8665269 - Flags: approval-mozilla-beta+
Attachment #8665269 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.