Bug 190590 – [MSE] timestampOffset can introduce floating-point rounding errors to incoming samples

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[MSE] timestampOffset can introduce floating-point rounding errors to incoming samples

[Radar WebKit Bug Importer]

<rdar://problem/45275626>

[Jer Noble]

Created attachment 352342 [details]
Patch

[EWS Watchlist]

Comment on attachment 352342 [details]
Patch

Attachment 352342 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9590165

New failing tests:
media/media-source/media-source-sequence-timestamps.html
media/media-source/media-source-timeoffset.html

[EWS Watchlist]

Created attachment 352347 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352342 [details]
Patch

Attachment 352342 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9590399

New failing tests:
media/media-source/media-source-sequence-timestamps.html
media/media-source/media-source-timeoffset.html

[EWS Watchlist]

Created attachment 352353 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352342 [details]
Patch

Attachment 352342 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9591336

New failing tests:
media/media-source/media-source-sequence-timestamps.html
media/media-source/media-source-timeoffset.html

[EWS Watchlist]

Created attachment 352362 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Jer Noble]

Created attachment 352365 [details]
Patch

[EWS Watchlist]

Comment on attachment 352365 [details]
Patch

Attachment 352365 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9595334

New failing tests:
media/media-source/media-source-sequence-timestamps.html

[EWS Watchlist]

Created attachment 352376 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352365 [details]
Patch

Attachment 352365 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9595524

New failing tests:
media/media-source/media-source-sequence-timestamps.html

[EWS Watchlist]

Created attachment 352377 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352365 [details]
Patch

Attachment 352365 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9596288

New failing tests:
media/media-source/media-source-sequence-timestamps.html

[EWS Watchlist]

Created attachment 352387 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Alicia Boya García]

Comment on attachment 352365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352365&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1485
> +                if (timeScale > std::numeric_limits<uint32_t>::max() / 2)

MediaTime::MaximumTimeScale should be used instead

[Jer Noble]

Created attachment 352500 [details]
Patch

[Alicia Boya García]

Comment on attachment 352500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352500&action=review

Nits aside, LGTM.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1424
> +        // NOTE: this is out-of-order, but we need the timescale from the

Is it worth it to reorder the code? We colud use `sample.duration` instead of `frameDuration`.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1499
> +            if (!trackBuffer.roundedTimestampOffset.isValid() || presentationTimestamp.timeScale() != trackBuffer.lastFrameTimescale) {

A TrackBuffer with samples with changing timescales is very rare. Should we add a mock test? (e.g. another one similar to our current test but where the second append has a different timescale)

> LayoutTests/media/media-source/media-source-timestampoffset-rounding-error.html:4
> +    <title>media-source-append-acb-tolerance</title>

media-source-timestampoffset-rounding-error

> LayoutTests/media/media-source/media-source-timestampoffset-rounding-error.html:50
> +        run('sourceBuffer.appendBuffer(makeVideo(2))');

makeVideo(1)

[Alicia Boya García]

Let me check the algorithm once more...

[Alicia Boya García]

Comment on attachment 352500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352500&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1489
> +                if (timeScale > MediaTime::MaximumTimeScale / 2)

This / 2 should not be there, since otherwise in the (hypothetical) case where roundingMargin is 1/MaximumTimeScale and we only need one multiplication to get there we would not do it even if it's helpful.

Even then, this would be problematic if MaximumTimescale >= 2^31, since doubling would cause overflow. We have a patch to make it 1e9 (which happens to be slightly < 2^31), but this assumption is not obvious from reading the algorithm.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1492
> +            } while (abs(roundedTime - time) > roundingMargin);

I would argue that this should be >=. The idea is that if we consider microsecond precision we ignore differences smaller than 1 us: conversely 1 us is noticeable enough to make a difference.

[Alicia Boya García]

Here is the algorithm modified to fix these problems, making no assumptions about the values of roundingMargin and MaximumTimeScale:

auto roundTowardsTimeScaleWithRoundingMargin = [] (const MediaTime& time, uint32_t timeScale, const MediaTime& roundingMargin) {
    while (true) {
        MediaTime roundedTime = time.toTimeScale(timeScale);
        if (abs(roundedTime - time) < roundingMargin || timeScale >= MediaTime::MaximumTimeScale)
            return roundedTime;

        timeScale = std::min<uint64_t>(static_cast<uint64_t>(timeScale) * 2, MediaTime::MaximumTimeScale);
    }
};

[Jer Noble]

(In reply to Alicia Boya García from comment #21)
> Here is the algorithm modified to fix these problems, making no assumptions
> about the values of roundingMargin and MaximumTimeScale:
> 
> auto roundTowardsTimeScaleWithRoundingMargin = [] (const MediaTime& time,
> uint32_t timeScale, const MediaTime& roundingMargin) {
>     while (true) {
>         MediaTime roundedTime = time.toTimeScale(timeScale);
>         if (abs(roundedTime - time) < roundingMargin || timeScale >=
> MediaTime::MaximumTimeScale)
>             return roundedTime;
> 
>         timeScale = std::min<uint64_t>(static_cast<uint64_t>(timeScale) * 2,
> MediaTime::MaximumTimeScale);
>     }
> };

Thanks for this! We have nice convenience methods for doing checked arithmetic like this; I'll see if I can update your algorithm in terms of CheckedArithmetic.h.

[Jer Noble]

(In reply to Alicia Boya García from comment #18)
> Comment on attachment 352500 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352500&action=review
> 
> Nits aside, LGTM.
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1424
> > +        // NOTE: this is out-of-order, but we need the timescale from the
> 
> Is it worth it to reorder the code? We colud use `sample.duration` instead
> of `frameDuration`.

True, but since we have to call sample.duration() already to create frameDuration, it would be a shame to have to call it twice.

> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1499
> > +            if (!trackBuffer.roundedTimestampOffset.isValid() || presentationTimestamp.timeScale() != trackBuffer.lastFrameTimescale) {
> 
> A TrackBuffer with samples with changing timescales is very rare. Should we
> add a mock test? (e.g. another one similar to our current test but where the
> second append has a different timescale)

Sure, we could do that. Since trackBuffers are not recreated across variants, timeScale changes are definitely possible, if rare.

> > LayoutTests/media/media-source/media-source-timestampoffset-rounding-error.html:4
> > +    <title>media-source-append-acb-tolerance</title>
> 
> media-source-timestampoffset-rounding-error

Whoops.

> > LayoutTests/media/media-source/media-source-timestampoffset-rounding-error.html:50
> > +        run('sourceBuffer.appendBuffer(makeVideo(2))');
> 
> makeVideo(1)

Changed.

[Jer Noble]

Created attachment 352712 [details]
Patch

[Eric Carlson]

Comment on attachment 352712 [details]
Patch

rs=me based on Alicia's review.

[WebKit Commit Bot]

Comment on attachment 352712 [details]
Patch

Clearing flags on attachment: 352712

Committed r237274: <https://trac.webkit.org/changeset/237274>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.