[GStreamer] Rework WebKitWebSrc to improve robustness
Created attachment 387224 [details] Patch
Comment on attachment 387224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387224&action=review > Source/WebCore/ChangeLog:11 > + waiting for more buffers that could be retrieved with the those typo, that type, the those > Source/WebCore/ChangeLog:13 > + the API that is not marked as fast. that are not > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2618 > + if ((m_isBuffering && !m_isLiveStream) || !m_playbackRate) { Why do we not pause buffering live streams? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361 > + " (min %u)", boolForPrinting(priv->isDownloadSuspended), boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->haveSize) why do we format MAX SIZE in min %u? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:363 > + if (priv->doesHaveEOS || !priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE) { s/doesHaveEOS/haveEOS/ I understand the EOS condition, but it's not clear why the absence of a Content-Length, not being seekable and/or being a small resource forbids us from stopping/restarting a download. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:374 > + GST_TRACE_OBJECT(src, "queue size %" G_GUINT64_FORMAT " (min %1.0lf)", queueSize I don't think you need %lf, just %f is fine? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:375 > + , priv->size * HIGH_QUEUE_FACTOR_THRESHOLD * LOW_QUEUE_FACTOR_THRESHOLD); This seems like an uncharacteristic line break for you :-) > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:399 > + , SMALL_MEDIA_RESOURCE_MAX_SIZE); Ditto MIN/MAX question > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:400 > + if (!priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE) { Why can we stop/restart in haveEOS now, but not in restartLoaderIfNeeded? There's duplicated checks in these mentioned methods you should take care of. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:415 > + priv->downloadStartTime = WallTime(); This looks like it is downloadStopTime, not start time... > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429 > GST_DEBUG_OBJECT(src, "Seeking, flushing adapter"); We are clearing now, not flushing. There seems to be some differences in behaviour here that are not obvious, particularly around pts/dts distance accounting the gstadapter. I don't know what effects this might have. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:450 > + // We don't read the GstAdapter methods marked as fast anymore because sometimes it was slower as we were waiting for more buffers that could be than could be > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:451 > + // retrieved with the those methods to be available and we had enough if we retrieved it with the API that is not marked as fast. error, the those. Please reword the comment. Also it's not clear what parts of this method the comment is even tied to anymore. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:453 > + , boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->isDownloadSuspended)); Again, I'm not sure what our code style is, I get told to put these all on one line, but I prefer breaking like this, so.... > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:474 > + GST_TRACE_OBJECT(src, "available %" G_GSIZE_FORMAT, queueSize); available BYTES, helps log readers > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:481 > + // 1. We are the end of the known size of the file. We are at the end of the file with a known size > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:482 > + // 2. Download is not suspended and no more data is arriving. We cannot wait forever, 10x1s seems safe and sensible. The download, more data are arriving > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:487 > + GST_TRACE_OBJECT(src, "is download suspended %s, tried %u", boolForPrinting(priv->isDownloadSuspended), retries + 1); isDownloadSuspended? %s, num retries: %u (makes it easier on the log reader) > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1152 > + // that first package delivery would include the time of sending out the request and getting the data back. Since we can't distingüish the encoding issue > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1157 > + GST_TRACE_OBJECT(src, "downloaded %u bytes in %lf seconds =~ %1.0lf bytes/second", priv->totalDownloadedBytes, timeSinceStart, priv->totalDownloadedBytes / timeSinceStart); I'm always nervous of unchecked division by zero, not matter how unlikely. Also the downloadStartTime being set in stopLoaderIfNecessary is smelling off here. Also %f instead of %lf
Comment on attachment 387224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387224&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2618 >> + if ((m_isBuffering && !m_isLiveStream) || !m_playbackRate) { > > Why do we not pause buffering live streams? This fixes a bug that was introduced during restyling. I think I found an issue during testing and realized of the regression. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:361 >> + " (min %u)", boolForPrinting(priv->isDownloadSuspended), boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->haveSize) > > why do we format MAX SIZE in min %u? I think we name of the constant is misleading. For me is the MINIMAL size under which we don't stop/restart. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:363 >> + if (priv->doesHaveEOS || !priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE) { > > s/doesHaveEOS/haveEOS/ > > I understand the EOS condition, but it's not clear why the absence of a Content-Length, not being seekable and/or being a small resource forbids us from stopping/restarting a download. In the case of a small resource it is just a matter of not bothering. If it's small we load it all and are happy. The case of being seekable is because we need to specify which range we want and if we can't do range requests, we can't seek. These checks where already in the old code I just refactored it. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:375 >> + , priv->size * HIGH_QUEUE_FACTOR_THRESHOLD * LOW_QUEUE_FACTOR_THRESHOLD); > > This seems like an uncharacteristic line break for you :-) Right? :) My limit is 150 chars. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:400 >> + if (!priv->haveSize || !priv->isSeekable || priv->size <= SMALL_MEDIA_RESOURCE_MAX_SIZE) { > > Why can we stop/restart in haveEOS now, but not in restartLoaderIfNeeded? There's duplicated checks in these mentioned methods you should take care of. Again, these checks were already in the original code, I just moved them around. For me the sense of this is that when stopping we can't (or shouldn't) be in EOS as we are downloading. In the case of restart, if we already signalled EOS to the pipeline so it does not make sense to push any more data downstream so we are going to abort any restarting attempts. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:415 >> + priv->downloadStartTime = WallTime(); > > This looks like it is downloadStopTime, not start time... No, it's start time. It's the time we consider we started the download. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:429 >> GST_DEBUG_OBJECT(src, "Seeking, flushing adapter"); > > We are clearing now, not flushing. There seems to be some differences in behaviour here that are not obvious, particularly around pts/dts distance accounting the gstadapter. I don't know what effects this might have. queueSize matches gst_adapter_available, which is the total size of bytes stored in it. Flushing the queuSize is the same as clearing and that's why I also removed queueSize, it was redundant and even gst_adapter_available is quick to call. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:453 >> + , boolForPrinting(priv->doesHaveEOS), boolForPrinting(priv->isDownloadSuspended)); > > Again, I'm not sure what our code style is, I get told to put these all on one line, but I prefer breaking like this, so.... The style does not set any fixed size, only something like "they should fit a reasonably screen size". I set my limit to 150.
Informal r+ from then.
Created attachment 387243 [details] Patch
Comment on attachment 387243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387243&action=review > Source/WebCore/ChangeLog:15 > + Remove this extra blank line please. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:148 > + WallTime downloadStartTime { WallTime() }; Is the default constructor here really wise to use? I mean, the download hasn't started exactly when this struct is being initialized, right? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:149 > + guint totalDownloadedBytes { 0 }; Shouldn't we use uint64_t here?
Comment on attachment 387243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387243&action=review >> Source/WebCore/ChangeLog:15 >> + > > Remove this extra blank line please. Sure. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:148 >> + WallTime downloadStartTime { WallTime() }; > > Is the default constructor here really wise to use? I mean, the download hasn't started exactly when this struct is being initialized, right? No, this is invalid, it will be initialized to now the first time we receive data. >> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:149 >> + guint totalDownloadedBytes { 0 }; > > Shouldn't we use uint64_t here? Yes.
Comment on attachment 387243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387243&action=review >>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:148 >>> + WallTime downloadStartTime { WallTime() }; >> >> Is the default constructor here really wise to use? I mean, the download hasn't started exactly when this struct is being initialized, right? > > No, this is invalid, it will be initialized to now the first time we receive data. Perhaps use WallTime::nan() then?
Created attachment 387511 [details] Patch
Comment on attachment 387511 [details] Patch Clearing flags on attachment: 387511 Committed r254503: <https://trac.webkit.org/changeset/254503>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58562312>