180258 2017-12-01 10:14:40 -0800 [MSE] Add isValid() check before using trackBuffer.lastEnqueuedPresentationTime 2017-12-14 15:14:27 -0800 1 1 1 Unclassified WebKit Media WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 aboya webkit-unassigned commit-queue jer.noble webkit-bug-importer oldest_to_newest 1376873 0 aboya 2017-12-01 10:14:40 -0800 The current comparison in sourceBufferPrivateDidReceiveSample() does not take in account the possibility of lastEnqueuedPresentationTime not being set. MediaTime currentMediaTime = m_source->currentTime(); if (currentMediaTime < trackBuffer.lastEnqueuedPresentationTime) { PlatformTimeRanges possiblyEnqueuedRanges(currentMediaTime, trackBuffer.lastEnqueuedPresentationTime); possiblyEnqueuedRanges.intersectWith(erasedRanges); if (possiblyEnqueuedRanges.length()) trackBuffer.needsReenqueueing = true; } Is this intentional? When experimenting with a new frame removal algorithm I hit a case where it was unset and therefore it created an invalid range in possiblyEnqueuedRanges. 1376876 1 328114 aboya 2017-12-01 10:16:41 -0800 Created attachment 328114 Patch 1376960 2 328114 jer.noble 2017-12-01 12:07:09 -0800 Comment on attachment 328114 Patch Is this change fix something, and if so, should it have a test? 1377657 3 aboya 2017-12-04 04:26:33 -0800 No.(In reply to Jer Noble from comment #2) > Comment on attachment 328114 [details] > Patch > > Is this change fix something, and if so, should it have a test? I'm not sure, that's why I'm asking if the assumption in the `if` above was intentional. As I see it, if is not intentional (i.e. it's possible that lastEnqueuedPresentationTime is MediaTime::invalidTime()) then we should check for that as in the patch. If it's intentional (i.e. it can never be invalid time), it's not obvious and should have an ASSERT() to make it more evident and avoid unexpected side effects should that ever cease being the case. 1381579 4 aboya 2017-12-14 04:25:56 -0800 Could we take a decision on this? 1381856 5 328114 commit-queue 2017-12-14 15:13:45 -0800 Comment on attachment 328114 Patch Clearing flags on attachment: 328114 Committed r225936: <https://trac.webkit.org/changeset/225936> 1381857 6 commit-queue 2017-12-14 15:13:46 -0800 All reviewed patches have been landed. Closing bug. 1381859 7 webkit-bug-importer 2017-12-14 15:14:27 -0800 <rdar://problem/36059745> 328114 2017-12-01 10:16:41 -0800 2017-12-14 15:13:45 -0800 Patch bug-180258-20171201191640.patch text/plain 1876 aboya U3VidmVyc2lvbiBSZXZpc2lvbjogMjI0MjI4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTkzZGMzOGM2ODU1NzZk YzJiNjYxMDgxNzUyY2JiMWYwMTFjYmMwNy4uZGY1ODU0OGE5OGMyOGUwMmQ0NDU4OWFmNzRkZDQx ZTRmZmY3ZjgxMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE3LTEyLTAxICBBbGlj aWEgQm95YSBHYXJjw61hICA8YWJveWFAaWdhbGlhLmNvbT4KKworICAgICAgICBbTVNFXSBBZGQg aXNWYWxpZCgpIGNoZWNrIGJlZm9yZSB1c2luZyB0cmFja0J1ZmZlci5sYXN0RW5xdWV1ZWRQcmVz ZW50YXRpb25UaW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xODAyNTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIE1vZHVsZXMvbWVkaWFzb3VyY2UvU291cmNlQnVmZmVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OlNvdXJjZUJ1ZmZlcjo6c291cmNlQnVmZmVyUHJpdmF0ZURpZFJlY2VpdmVTYW1wbGUp OgorCiAyMDE3LTEyLTAxICBBbGljaWEgQm95YSBHYXJjw61hICA8YWJveWFAaWdhbGlhLmNvbT4K IAogICAgICAgICBbTVNFXSBBZGQgaXNWYWxpZCgpIGFzc2VydGlvbnMgb24gUGxhdGZvcm1UaW1l UmFuZ2VzOjphZGQoKSByYW5nZSBlbmRzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1 bGVzL21lZGlhc291cmNlL1NvdXJjZUJ1ZmZlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVz L21lZGlhc291cmNlL1NvdXJjZUJ1ZmZlci5jcHAKaW5kZXggZWY0N2MzMGU3NDQwMWNlMWU1Yjhi MDE5YTcxZDNhM2YwZjBmOGQ4Ni4uYWZiOWZlODc4MWJiY2VmZmJmNmE2ZWFiYTE2MjUwZjJhOTFm Y2ViZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNvdXJjZS9Tb3Vy Y2VCdWZmZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzb3VyY2UvU291 cmNlQnVmZmVyLmNwcApAQCAtMTU2Niw3ICsxNTY2LDcgQEAgdm9pZCBTb3VyY2VCdWZmZXI6OnNv dXJjZUJ1ZmZlclByaXZhdGVEaWRSZWNlaXZlU2FtcGxlKE1lZGlhU2FtcGxlJiBzYW1wbGUpCiAg ICAgICAgICAgICAvLyBPbmx5IGZvcmNlIHRoZSBUcmFja0J1ZmZlciB0byByZS1lbnF1ZXVlIGlm IHRoZSByZW1vdmVkIHJhbmdlcyBvdmVybGFwIHdpdGggZW5xdWV1ZWQgYW5kIHBvc3NpYmx5CiAg ICAgICAgICAgICAvLyBub3QgeWV0IGRpc3BsYXllZCBzYW1wbGVzLgogICAgICAgICAgICAgTWVk aWFUaW1lIGN1cnJlbnRNZWRpYVRpbWUgPSBtX3NvdXJjZS0+Y3VycmVudFRpbWUoKTsKLSAgICAg ICAgICAgIGlmIChjdXJyZW50TWVkaWFUaW1lIDwgdHJhY2tCdWZmZXIubGFzdEVucXVldWVkUHJl c2VudGF0aW9uVGltZSkgeworICAgICAgICAgICAgaWYgKHRyYWNrQnVmZmVyLmxhc3RFbnF1ZXVl ZFByZXNlbnRhdGlvblRpbWUuaXNWYWxpZCgpICYmIGN1cnJlbnRNZWRpYVRpbWUgPCB0cmFja0J1 ZmZlci5sYXN0RW5xdWV1ZWRQcmVzZW50YXRpb25UaW1lKSB7CiAgICAgICAgICAgICAgICAgUGxh dGZvcm1UaW1lUmFuZ2VzIHBvc3NpYmx5RW5xdWV1ZWRSYW5nZXMoY3VycmVudE1lZGlhVGltZSwg dHJhY2tCdWZmZXIubGFzdEVucXVldWVkUHJlc2VudGF0aW9uVGltZSk7CiAgICAgICAgICAgICAg ICAgcG9zc2libHlFbnF1ZXVlZFJhbmdlcy5pbnRlcnNlY3RXaXRoKGVyYXNlZFJhbmdlcyk7CiAg ICAgICAgICAgICAgICAgaWYgKHBvc3NpYmx5RW5xdWV1ZWRSYW5nZXMubGVuZ3RoKCkpCg==