174505 2017-07-14 09:26:13 -0700 [MediaStream] Limit the number of remote video samples queued 2017-07-14 11:35:03 -0700 1 1 1 Unclassified WebKit Media Other Unspecified Unspecified ASSIGNED InRadar P2 Normal --- 1 eric.carlson eric.carlson commit-queue webkit-bug-importer youennf oldest_to_newest 1328799 0 eric.carlson 2017-07-14 09:26:13 -0700 MediaPlayerPrivateMediaStreamAVFObjC::removeOldSamplesFromPendingQueue releases queued video samples that are earlier than the current stream time. This works for samples from a local media stream track, but frames from a remote track are tagged for "immediate display" and don't have valid time stamps so there is no such thing as an "old" sample. 1328801 1 eric.carlson 2017-07-14 09:28:57 -0700 <rdar://problem/33223015> 1328810 2 315435 eric.carlson 2017-07-14 09:40:36 -0700 Created attachment 315435 Proposed patch. 1328835 3 315435 youennf 2017-07-14 10:23:01 -0700 Comment on attachment 315435 Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=315435&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:263 > + if (!queue.first()->decodeTime().isValid()) { I applied the patch locally to check whether decodeTime is invalid in the case of RealtimeIncomingVideoSource. It is valid but is negative. At least on my Mac, we are not going through that loop. We will probably go through the next while loop and empty the queue there. Is it different on iOS? 1328857 4 eric.carlson 2017-07-14 10:50:45 -0700 (In reply to youenn fablet from comment #3) > Comment on attachment 315435 [details] > Proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315435&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:263 > > + if (!queue.first()->decodeTime().isValid()) { > > I applied the patch locally to check whether decodeTime is invalid in the > case of RealtimeIncomingVideoSource. > It is valid but is negative. At least on my Mac, we are not going through > that loop. > We will probably go through the next while loop and empty the queue there. > Is it different on iOS? While the raw PTS time is negative in the CMSampleBuffer: (lldb) po (CMSampleBuffer)0x00007f88b20ca290 CMSampleBuffer 0x7f88b20ca290 retainCount: 1 allocator: 0x7fff8383df40 ... numSamples = 1 sampleTimingArray[1] = { {PTS = {-1/1 = -1.000}, DTS = {-1/1 = -1.000}, duration = {INVALID}}, } sampleAttachmentsArray[0] = { sample 0: DisplayImmediately = true } imageBuffer = 0x7f88b5c2f140 "decodeTime" is invalid: (lldb) p mediaSample.decodeTime() (WTF::MediaTime) $11 = { Invalid } { = (m_timeValue = 0, m_timeValueAsDouble = 0) m_timeScale = 0 m_timeFlags = '\0' } (lldb) p mediaSample.decodeTime().isValid() (bool) $12 = false But we should probably also check explicitly for negative times as well. 1328878 5 315457 eric.carlson 2017-07-14 11:12:39 -0700 Created attachment 315457 Patch for landing. 1328906 6 315457 commit-queue 2017-07-14 11:35:03 -0700 Comment on attachment 315457 Patch for landing. Clearing flags on attachment: 315457 Committed r219513: <http://trac.webkit.org/changeset/219513> 315435 2017-07-14 09:40:36 -0700 2017-07-14 11:13:14 -0700 Proposed patch. limit_sample_queue_patch_1.txt text/plain 1920 eric.carlson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxOTUwNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE3LTA3LTE0ICBFcmljIENh cmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUuY29tPgorCisgICAgICAgIFtNZWRpYVN0cmVhbV0g TGltaXQgdGhlIG51bWJlciBvZiByZW1vdGUgdmlkZW8gc2FtcGxlcyBxdWV1ZWQKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NDUwNQorICAgICAgICA8 cmRhcjovL3Byb2JsZW0vMzMyMjMwMTU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9hdmZvdW5kYXRpb24vb2JqYy9N ZWRpYVBsYXllclByaXZhdGVNZWRpYVN0cmVhbUFWRk9iakMubW06CisgICAgICAgIChXZWJDb3Jl OjpNZWRpYVBsYXllclByaXZhdGVNZWRpYVN0cmVhbUFWRk9iakM6OnJlbW92ZU9sZFNhbXBsZXNG cm9tUGVuZGluZ1F1ZXVlKTogT25seQorICAgICAgICBlbnF1ZXVlIGEgZml4ZWQgbnVtYmVyIG9m IGZyYW1lcyB3aXRoIGludmFsaWQgZGVjb2RlIHRpbWVzLgorCiAyMDE3LTA3LTE0ICBGdWppaSBI aXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgogCiAgICAgICAgIFtXaW5DYWlyb10g ZXJyb3IgJ21fY29tcG9zaXRvclRleHR1cmUnOiB1bmRlY2xhcmVkIGlkZW50aWZpZXIgc2luY2Ug QnVnIDE3NDM0NQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZmb3Vu ZGF0aW9uL29iamMvTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDLm1tCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2F2Zm91bmRhdGlvbi9v YmpjL01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQy5tbQkocmV2aXNpb24gMjE5 NTAzKQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZmb3VuZGF0aW9uL29i amMvTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDLm1tCSh3b3JraW5nIGNvcHkp CkBAIC0yNTcsMTIgKzI1NywyMiBAQCBNZWRpYVBsYXllcjo6U3VwcG9ydHNUeXBlIE1lZGlhUGxh eWVyUHJpCiAKIHZvaWQgTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDOjpyZW1v dmVPbGRTYW1wbGVzRnJvbVBlbmRpbmdRdWV1ZShQZW5kaW5nU2FtcGxlUXVldWUmIHF1ZXVlKQog eworICAgIGlmIChxdWV1ZS5pc0VtcHR5KCkpCisgICAgICAgIHJldHVybjsKKworICAgIGlmICgh cXVldWUuZmlyc3QoKS0+ZGVjb2RlVGltZSgpLmlzVmFsaWQoKSkgeworICAgICAgICB3aGlsZSAo cXVldWUuc2l6ZSgpID4gNSkKKyAgICAgICAgICAgIHF1ZXVlLnJlbW92ZUZpcnN0KCk7CisKKyAg ICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIE1lZGlhVGltZSBub3cgPSBzdHJlYW1UaW1lKCk7 CiAgICAgd2hpbGUgKCFxdWV1ZS5pc0VtcHR5KCkpIHsKICAgICAgICAgaWYgKHF1ZXVlLmZpcnN0 KCktPmRlY29kZVRpbWUoKSA+IG5vdykKICAgICAgICAgICAgIGJyZWFrOwogICAgICAgICBxdWV1 ZS5yZW1vdmVGaXJzdCgpOwotICAgIH07CisgICAgfQogfQogCiB2b2lkIE1lZGlhUGxheWVyUHJp dmF0ZU1lZGlhU3RyZWFtQVZGT2JqQzo6YWRkU2FtcGxlVG9QZW5kaW5nUXVldWUoUGVuZGluZ1Nh bXBsZVF1ZXVlJiBxdWV1ZSwgTWVkaWFTYW1wbGUmIHNhbXBsZSkK 315457 2017-07-14 11:12:39 -0700 2017-07-14 11:35:03 -0700 Patch for landing. limit_sample_queue_patch_2.txt text/plain 2004 eric.carlson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxOTUwNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE3LTA3LTE0ICBFcmljIENh cmxzb24gIDxlcmljLmNhcmxzb25AYXBwbGUuY29tPgorCisgICAgICAgIFtNZWRpYVN0cmVhbV0g TGltaXQgdGhlIG51bWJlciBvZiByZW1vdGUgdmlkZW8gc2FtcGxlcyBxdWV1ZWQKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NDUwNQorICAgICAgICA8 cmRhcjovL3Byb2JsZW0vMzMyMjMwMTU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgWW91ZW5uIEZh YmxldC4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2F2Zm91bmRhdGlvbi9vYmpjL01l ZGlhUGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQy5tbToKKyAgICAgICAgKFdlYkNvcmU6 Ok1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQzo6cmVtb3ZlT2xkU2FtcGxlc0Zy b21QZW5kaW5nUXVldWUpOiBPbmx5CisgICAgICAgIGVucXVldWUgYSBmaXhlZCBudW1iZXIgb2Yg ZnJhbWVzIHdpdGggaW52YWxpZCBvciBuZWdhdGl2ZSBkZWNvZGUgdGltZXMuCisKIDIwMTctMDct MTQgIEZ1amlpIEhpcm9ub3JpICA8SGlyb25vcmkuRnVqaWlAc29ueS5jb20+CiAKICAgICAgICAg W1dpbkNhaXJvXSBlcnJvciAnbV9jb21wb3NpdG9yVGV4dHVyZSc6IHVuZGVjbGFyZWQgaWRlbnRp ZmllciBzaW5jZSBCdWcgMTc0MzQ1CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9hdmZvdW5kYXRpb24vb2JqYy9NZWRpYVBsYXllclByaXZhdGVNZWRpYVN0cmVhbUFWRk9i akMubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZm b3VuZGF0aW9uL29iamMvTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFTdHJlYW1BVkZPYmpDLm1tCShy ZXZpc2lvbiAyMTk1MDMpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9hdmZv dW5kYXRpb24vb2JqYy9NZWRpYVBsYXllclByaXZhdGVNZWRpYVN0cmVhbUFWRk9iakMubW0JKHdv cmtpbmcgY29weSkKQEAgLTI1NywxMiArMjU3LDIzIEBAIE1lZGlhUGxheWVyOjpTdXBwb3J0c1R5 cGUgTWVkaWFQbGF5ZXJQcmkKIAogdm9pZCBNZWRpYVBsYXllclByaXZhdGVNZWRpYVN0cmVhbUFW Rk9iakM6OnJlbW92ZU9sZFNhbXBsZXNGcm9tUGVuZGluZ1F1ZXVlKFBlbmRpbmdTYW1wbGVRdWV1 ZSYgcXVldWUpCiB7CisgICAgaWYgKHF1ZXVlLmlzRW1wdHkoKSkKKyAgICAgICAgcmV0dXJuOwor CisgICAgYXV0byBkZWNvZGVUaW1lID0gcXVldWUuZmlyc3QoKS0+ZGVjb2RlVGltZSgpOworICAg IGlmICghZGVjb2RlVGltZS5pc1ZhbGlkKCkgfHwgZGVjb2RlVGltZSA8IE1lZGlhVGltZTo6emVy b1RpbWUoKSkgeworICAgICAgICB3aGlsZSAocXVldWUuc2l6ZSgpID4gNSkKKyAgICAgICAgICAg IHF1ZXVlLnJlbW92ZUZpcnN0KCk7CisKKyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIE1l ZGlhVGltZSBub3cgPSBzdHJlYW1UaW1lKCk7CiAgICAgd2hpbGUgKCFxdWV1ZS5pc0VtcHR5KCkp IHsKICAgICAgICAgaWYgKHF1ZXVlLmZpcnN0KCktPmRlY29kZVRpbWUoKSA+IG5vdykKICAgICAg ICAgICAgIGJyZWFrOwogICAgICAgICBxdWV1ZS5yZW1vdmVGaXJzdCgpOwotICAgIH07CisgICAg fQogfQogCiB2b2lkIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhU3RyZWFtQVZGT2JqQzo6YWRkU2Ft cGxlVG9QZW5kaW5nUXVldWUoUGVuZGluZ1NhbXBsZVF1ZXVlJiBxdWV1ZSwgTWVkaWFTYW1wbGUm IHNhbXBsZSkK