214252 2020-07-13 02:59:49 -0700 [MSE][GStreamer] Discard PTS-less samples 2020-07-13 06:03:28 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 aboya aboya bugs-noreply calvaris cgarcia ews-watchlist gustavo menard pnormand vjaquez oldest_to_newest 1670995 0 aboya 2020-07-13 02:59:49 -0700 In some cases GStreamer demuxers emit PTS-less samples with metadata at the beginning of the container. These are fortunately not necessary for playback, and in fact incompatible with the way MSE works, where you should be able to start playing a stream from the middle. This patch skips these frames in the AppendPipeline so they don't pollute other parts of the MSE codebase. Since these frames were not necessary and were later overwritten, this patch is just a cleanup introducing no notable behavior changes. 1670996 1 404138 aboya 2020-07-13 03:02:32 -0700 Created attachment 404138 Patch 1670997 2 aboya 2020-07-13 03:04:12 -0700 This a cherry picked change from the WebKitMediaSrc rework patch. 1671001 3 404138 pnormand 2020-07-13 03:25:34 -0700 Comment on attachment 404138 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404138&action=review > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464 > + // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those. If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination? 1671002 4 404138 aboya 2020-07-13 04:17:07 -0700 Comment on attachment 404138 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404138&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464 >> + // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those. > > If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination? Not really. Imagine there was another container+format case where this happens, skipping these frames (which are MSE-incompatible) would still be the best outcome. Adding a further check would add more complexity for no gain: regardless of the format where they are emitted, the root problem is they are "metadata" samples without a presentation time. 1671003 5 404138 pnormand 2020-07-13 04:23:07 -0700 Comment on attachment 404138 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404138&action=review >>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464 >>> + // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those. >> >> If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination? > > Not really. Imagine there was another container+format case where this happens, skipping these frames (which are MSE-incompatible) would still be the best outcome. Adding a further check would add more complexity for no gain: regardless of the format where they are emitted, the root problem is they are "metadata" samples without a presentation time. I don't really agree, I prefer to be explicit in this case. But I don't have the energy to discuss about this. Anyway, this doesn't impact live streams broadcasted on youtube.com/live? 1671004 6 404138 aboya 2020-07-13 04:44:25 -0700 Comment on attachment 404138 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404138&action=review >>>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:464 >>>> + // When demuxing Vorbis, matroskademux creates several PTS-less frames with header information. We don't need those. >>> >>> If this is related with a specific container/codec, would it make sense to check the sample caps and proceed only for this combination? >> >> Not really. Imagine there was another container+format case where this happens, skipping these frames (which are MSE-incompatible) would still be the best outcome. Adding a further check would add more complexity for no gain: regardless of the format where they are emitted, the root problem is they are "metadata" samples without a presentation time. > > I don't really agree, I prefer to be explicit in this case. But I don't have the energy to discuss about this. > Anyway, this doesn't impact live streams broadcasted on youtube.com/live? I see no benefit in adding more conditions, for the reasons explained above. The only case in which it could make sense to me would be if I asserted on the caps inside the `if` block (e.g. this only happens for Vorbis streams), but it feels a bit overkill. It doesn't impact live streams. Live streams frames contain PTS just like regular videos do. All frames in MSE need a PTS. You can indeed open a developer console and inspect `$("video").buffered` in a YouTube live to see how this is the case. The only difference with live streams is that there is not necessarily a frame with PTS=0, implementations may make the live start at whatever PTS they want. 1671010 7 ews-feeder 2020-07-13 06:03:26 -0700 Committed r264299: <https://trac.webkit.org/changeset/264299> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404138. 404138 2020-07-13 03:02:32 -0700 2020-07-13 06:03:27 -0700 Patch bug-214252-20200713120231.patch text/plain 2336 aboya U3VidmVyc2lvbiBSZXZpc2lvbjogMjY0MjExCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWIwZTUxNTg0ZWJmNThi NmU2Nzk1MTU3MTNmNjk1NzNhNmYwZjE1Mi4uMDdlMjNjZDM0ODVhMDY3ZTA5MGJiYjgxMThiYWVj OTBhNjA3MjBiZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDIwLTA3LTEzICBBbGlj aWEgQm95YSBHYXJjw61hICA8YWJveWFAaWdhbGlhLmNvbT4KKworICAgICAgICBbTVNFXVtHU3Ry ZWFtZXJdIERpc2NhcmQgUFRTLWxlc3Mgc2FtcGxlcworICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE0MjUyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gc29tZSBjYXNlcyBHU3RyZWFtZXIgZGVtdXhlcnMg ZW1pdCBQVFMtbGVzcyBzYW1wbGVzIHdpdGggbWV0YWRhdGEKKyAgICAgICAgYXQgdGhlIGJlZ2lu bmluZyBvZiB0aGUgY29udGFpbmVyLiBUaGVzZSBhcmUgZm9ydHVuYXRlbHkgbm90IG5lY2Vzc2Fy eQorICAgICAgICBmb3IgcGxheWJhY2ssIGFuZCBpbiBmYWN0IGluY29tcGF0aWJsZSB3aXRoIHRo ZSB3YXkgTVNFIHdvcmtzLCB3aGVyZQorICAgICAgICB5b3Ugc2hvdWxkIGJlIGFibGUgdG8gc3Rh cnQgcGxheWluZyBhIHN0cmVhbSBmcm9tIHRoZSBtaWRkbGUuCisKKyAgICAgICAgVGhpcyBwYXRj aCBza2lwcyB0aGVzZSBmcmFtZXMgaW4gdGhlIEFwcGVuZFBpcGVsaW5lIHNvIHRoZXkgZG9uJ3QK KyAgICAgICAgcG9sbHV0ZSBvdGhlciBwYXJ0cyBvZiB0aGUgTVNFIGNvZGViYXNlLgorCisgICAg ICAgIFNpbmNlIHRoZXNlIGZyYW1lcyB3ZXJlIG5vdCBuZWNlc3NhcnkgYW5kIHdlcmUgbGF0ZXIg b3ZlcndyaXR0ZW4sCisgICAgICAgIHRoaXMgcGF0Y2ggaXMganVzdCBhIGNsZWFudXAgaW50cm9k dWNpbmcgbm8gbm90YWJsZSBiZWhhdmlvciBjaGFuZ2VzLgorCisgICAgICAgICogcGxhdGZvcm0v Z3JhcGhpY3MvZ3N0cmVhbWVyL21zZS9BcHBlbmRQaXBlbGluZS5jcHA6CisgICAgICAgIChXZWJD b3JlOjpBcHBlbmRQaXBlbGluZTo6YXBwc2lua05ld1NhbXBsZSk6CisKIDIwMjAtMDctMTAgIEFs aWNpYSBCb3lhIEdhcmPDrWEgIDxhYm95YUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtNU0VdW0dT dHJlYW1lcl0gSW5saW5lIE1lZGlhU291cmNlQ2xpZW50R1N0cmVhbWVyTVNFIGF3YXkKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dzdHJlYW1lci9tc2UvQXBw ZW5kUGlwZWxpbmUuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVh bWVyL21zZS9BcHBlbmRQaXBlbGluZS5jcHAKaW5kZXggNGE0MzY5NjJlN2ZiNTk0YWRmYTU2YjI1 NGM2MTgwZTcyN2JmNTc2ZS4uNzRjMzk4NzAzNzllMjViNDk1MmYzN2YyZTgzMzMwNzUxNDk2YmQ3 OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVy L21zZS9BcHBlbmRQaXBlbGluZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3Jh cGhpY3MvZ3N0cmVhbWVyL21zZS9BcHBlbmRQaXBlbGluZS5jcHAKQEAgLTQ2MCw2ICs0NjAsMTIg QEAgdm9pZCBBcHBlbmRQaXBlbGluZTo6YXBwc2lua05ld1NhbXBsZShHUmVmUHRyPEdzdFNhbXBs ZT4mJiBzYW1wbGUpCiAgICAgICAgIHJldHVybjsKICAgICB9CiAKKyAgICBpZiAoIUdTVF9CVUZG RVJfUFRTX0lTX1ZBTElEKGdzdF9zYW1wbGVfZ2V0X2J1ZmZlcihzYW1wbGUuZ2V0KCkpKSkgewor ICAgICAgICAvLyBXaGVuIGRlbXV4aW5nIFZvcmJpcywgbWF0cm9za2FkZW11eCBjcmVhdGVzIHNl dmVyYWwgUFRTLWxlc3MgZnJhbWVzIHdpdGggaGVhZGVyIGluZm9ybWF0aW9uLiBXZSBkb24ndCBu ZWVkIHRob3NlLgorICAgICAgICBHU1RfREVCVUcoIklnbm9yaW5nIHNhbXBsZSB3aXRob3V0IFBU UzogJSIgR1NUX1BUUl9GT1JNQVQsIGdzdF9zYW1wbGVfZ2V0X2J1ZmZlcihzYW1wbGUuZ2V0KCkp KTsKKyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIGF1dG8gbWVkaWFTYW1wbGUgPSBXZWJD b3JlOjpNZWRpYVNhbXBsZUdTdHJlYW1lcjo6Y3JlYXRlKFdURk1vdmUoc2FtcGxlKSwgbV9wcmVz ZW50YXRpb25TaXplLCB0cmFja0lkKCkpOwogCiAgICAgR1NUX1RSQUNFKCJhcHBlbmQ6IHRyYWNr SWQ9JXMgUFRTPSVzIERUUz0lcyBEVVI9JXMgcHJlc2VudGF0aW9uU2l6ZT0lLjBmeCUuMGYiLAo=