[MSE] timestampOffset can introduce floating-point rounding errors to incoming samples
<rdar://problem/45275626>
Created attachment 352342 [details] Patch
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
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
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
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
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
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
Created attachment 352365 [details] Patch
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
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
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
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
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
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
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
Created attachment 352500 [details] Patch
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)
Let me check the algorithm once more...
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.
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); } };
(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.
(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.
Created attachment 352712 [details] Patch
Comment on attachment 352712 [details] Patch rs=me based on Alicia's review.
Comment on attachment 352712 [details] Patch Clearing flags on attachment: 352712 Committed r237274: <https://trac.webkit.org/changeset/237274>
All reviewed patches have been landed. Closing bug.