177951 2017-10-05 10:51:05 -0700 [MSE] Dead code in SourceBuffer::appendBufferTimerFired() 2017-10-06 02:22:08 -0700 1 1 1 Unclassified WebKit Media WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 eocanha eocanha aboya commit-queue eocanha jer.noble webkit-bug-importer zan oldest_to_newest 1357086 0 eocanha 2017-10-05 10:51:05 -0700 The second block of code here[1] is never going to be run because the first block makes the vector to be non-empty: size_t appendSize = m_pendingAppendData.size(); if (!appendSize) { // Resize buffer for 0 byte appends so we always have a valid pointer. // We need to convey all appends, even 0 byte ones to |m_private| so // that it can clear its end of stream state if necessary. m_pendingAppendData.resize(1); } // Section 3.5.1 Segment Parser Loop // https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#sourcebuffer-segment-parser-loop // When the segment parser loop algorithm is invoked, run the following steps: // 1. Loop Top: If the input buffer is empty, then jump to the need more data step below. if (!m_pendingAppendData.size()) { sourceBufferPrivateAppendComplete(AppendSucceeded); return; } Probably the whole platform layer append operation could be avoided by removing the first block and just completing the append in the second code block. Am I missing something? [1] https://github.com/WebKit/webkit/blob/624c264648af81a0cf6a3818db9d776d4d6ee15c/Source/WebCore/Modules/mediasource/SourceBuffer.cpp#L549 1357279 1 aboya 2017-10-05 15:18:02 -0700 The comment makes me suspect that `.resize(1)` should actually be `.reserveCapacity(1)` and that zero-length appends are not being handled correctly. 1357295 2 322926 eocanha 2017-10-05 15:44:40 -0700 Created attachment 322926 Patch 1357298 3 jer.noble 2017-10-05 15:54:16 -0700 This code was inherited from blink, and appears to have been totally re-written in blink since the merge.(In reply to Alicia Boya García from comment #1) > The comment makes me suspect that `.resize(1)` should actually be > `.reserveCapacity(1)` and that zero-length appends are not being handled > correctly. Seems to me that we can now change SourceBufferPrivate::append() to take a "Vector<unsigned char>&&" rather than a "void*, size_t". That would make this resize (or reserveCapacity) unnecessary, and also avoid a copy operation inside the private. 1357314 4 eocanha 2017-10-05 16:10:21 -0700 Yes, Zan wants to do that change in a different patch but he'd like to have this dead code issue sorted out before sending his patch. 1357499 5 322926 commit-queue 2017-10-06 00:43:56 -0700 Comment on attachment 322926 Patch Clearing flags on attachment: 322926 Committed r222965: <http://trac.webkit.org/changeset/222965> 1357500 6 commit-queue 2017-10-06 00:43:58 -0700 All reviewed patches have been landed. Closing bug. 1357504 7 webkit-bug-importer 2017-10-06 00:45:52 -0700 <rdar://problem/34851791> 1357524 8 zan 2017-10-06 02:22:08 -0700 (In reply to Enrique Ocaña from comment #4) > Yes, Zan wants to do that change in a different patch but he'd like to have > this dead code issue sorted out before sending his patch. See bug #178003. 322926 2017-10-05 15:44:40 -0700 2017-10-06 00:43:56 -0700 Patch bug-177951-20171005224439.patch text/plain 2221 eocanha U3VidmVyc2lvbiBSZXZpc2lvbjogMjIyNDk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMmQwNThhZmE4ODMwNjRj OTAzMjllM2I4YjYzZThiMTRlYTllNmJhMy4uNGVkN2NhYTIwYTdlMzQyZDZjZDZkMTJjMmRjYzEy NGE3NzJlZDk3OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE3LTEwLTA1ICBFbnJp cXVlIE9jYcOxYSBHb256w6FsZXogIDxlb2NhbmhhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW01T RV0gRGVhZCBjb2RlIGluIFNvdXJjZUJ1ZmZlcjo6YXBwZW5kQnVmZmVyVGltZXJGaXJlZCgpCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzc5NTEKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDb3ZlcmVkIGJ5 IExheW91dFRlc3RzL2ltcG9ydGVkL3czYy93ZWItcGxhdGZvcm0tdGVzdHMvbWVkaWEtc291cmNl L21lZGlhc291cmNlLWFwcGVuZC1idWZmZXIuaHRtbC4KKworICAgICAgICAqIE1vZHVsZXMvbWVk aWFzb3VyY2UvU291cmNlQnVmZmVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNvdXJjZUJ1ZmZl cjo6YXBwZW5kQnVmZmVyVGltZXJGaXJlZCk6IFJlbW92ZSByZWR1bmRhbnQgY29kZS4KKwogMjAx Ny0wOS0yNiAgWmFuIERvYmVyc2VrICA8emRvYmVyc2VrQGlnYWxpYS5jb20+CiAKICAgICAgICAg W0VNRV0gQWRkIENsZWFyS2V5IHN1cHBvcnQgZm9yIHBlcnNpc3RlbnQgc2Vzc2lvbiBkYXRhIGxv YWQgYW5kIHJlbW92YWwKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFz b3VyY2UvU291cmNlQnVmZmVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzb3Vy Y2UvU291cmNlQnVmZmVyLmNwcAppbmRleCA0MjRlMjQxM2FmOGMwMzEwZWU0NTlmYzMxM2ZjMjRk Y2Q1MmE3MWUzLi5lYTUyODI2MGNjZTUwYTkyOGRhNTIxMWI2NzEyOTlkODQ3M2U0ZDNjIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc291cmNlL1NvdXJjZUJ1ZmZlci5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNvdXJjZS9Tb3VyY2VCdWZmZXIu Y3BwCkBAIC01NDcsMTMgKzU0Nyw2IEBAIHZvaWQgU291cmNlQnVmZmVyOjphcHBlbmRCdWZmZXJU aW1lckZpcmVkKCkKICAgICAvLyBodHRwczovL2R2Y3MudzMub3JnL2hnL2h0bWwtbWVkaWEvcmF3 LWZpbGUvZGVmYXVsdC9tZWRpYS1zb3VyY2UvbWVkaWEtc291cmNlLmh0bWwjc291cmNlYnVmZmVy LWJ1ZmZlci1hcHBlbmQKIAogICAgIC8vIDEuIFJ1biB0aGUgc2VnbWVudCBwYXJzZXIgbG9vcCBh bGdvcml0aG0uCi0gICAgc2l6ZV90IGFwcGVuZFNpemUgPSBtX3BlbmRpbmdBcHBlbmREYXRhLnNp emUoKTsKLSAgICBpZiAoIWFwcGVuZFNpemUpIHsKLSAgICAgICAgLy8gUmVzaXplIGJ1ZmZlciBm b3IgMCBieXRlIGFwcGVuZHMgc28gd2UgYWx3YXlzIGhhdmUgYSB2YWxpZCBwb2ludGVyLgotICAg ICAgICAvLyBXZSBuZWVkIHRvIGNvbnZleSBhbGwgYXBwZW5kcywgZXZlbiAwIGJ5dGUgb25lcyB0 byB8bV9wcml2YXRlfCBzbwotICAgICAgICAvLyB0aGF0IGl0IGNhbiBjbGVhciBpdHMgZW5kIG9m IHN0cmVhbSBzdGF0ZSBpZiBuZWNlc3NhcnkuCi0gICAgICAgIG1fcGVuZGluZ0FwcGVuZERhdGEu cmVzaXplKDEpOwotICAgIH0KIAogICAgIC8vIFNlY3Rpb24gMy41LjEgU2VnbWVudCBQYXJzZXIg TG9vcAogICAgIC8vIGh0dHBzOi8vZHZjcy53My5vcmcvaGcvaHRtbC1tZWRpYS9yYXctZmlsZS90 aXAvbWVkaWEtc291cmNlL21lZGlhLXNvdXJjZS5odG1sI3NvdXJjZWJ1ZmZlci1zZWdtZW50LXBh cnNlci1sb29wCkBAIC01NjUsNyArNTU4LDcgQEAgdm9pZCBTb3VyY2VCdWZmZXI6OmFwcGVuZEJ1 ZmZlclRpbWVyRmlyZWQoKQogICAgICAgICByZXR1cm47CiAgICAgfQogCi0gICAgbV9wcml2YXRl LT5hcHBlbmQobV9wZW5kaW5nQXBwZW5kRGF0YS5kYXRhKCksIGFwcGVuZFNpemUpOworICAgIG1f cHJpdmF0ZS0+YXBwZW5kKG1fcGVuZGluZ0FwcGVuZERhdGEuZGF0YSgpLCBtX3BlbmRpbmdBcHBl bmREYXRhLnNpemUoKSk7CiAgICAgbV9wZW5kaW5nQXBwZW5kRGF0YS5jbGVhcigpOwogfQogCg==