| Summary: | [MSE][GStreamer] Discard PTS-less samples | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alicia Boya García <aboya> | ||||
| Component: | WebKitGTK | Assignee: | Alicia Boya García <aboya> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, ews-watchlist, gustavo, menard, pnormand, vjaquez | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Alicia Boya García
2020-07-13 02:59:49 PDT
Created attachment 404138 [details]
Patch
This a cherry picked change from the WebKitMediaSrc rework patch. Comment on attachment 404138 [details] 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? Comment on attachment 404138 [details] 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. Comment on attachment 404138 [details] 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? Comment on attachment 404138 [details] 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. Committed r264299: <https://trac.webkit.org/changeset/264299> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404138 [details]. |