The http/tests/media/hls/hls-progress.html layout test is failing since it was added in r179699 (http://trac.webkit.org/changeset/r179699). The diff is the following: --- /home/ch01/wk-tools/layout-test-results/http/tests/media/hls/hls-progress-expected.txt +++ /home/ch01/wk-tools/layout-test-results/http/tests/media/hls/hls-progress-actual.txt @@ -1,4 +1,4 @@ -EVENT(progress) +EVENT(stalled) TEST(false) FAIL END OF TEST
Drive by commenting while investigating something else. The issue is in the fillTImerFired function where it queries the pipeline for its buffering status. This always fails on HLS streams AFAICT. I haven't dug into why no element in the pipeline bin gives an acceptable response, but somewhere there lies the issue.
At the moment, GStreamer can't support the use case required by WebKit. HTMLMediaElement expects a progress event within three seconds. The only way we have of getting progress information is from buffering events at the moment. Unfortunately, for many real-world HLS streams, it takes longer than 3 seconds before those events even get sent. Our current strategy for progress information is to poll the pipeline with buffering queries. We can't poll the HLS pipeline for buffering information, because the hlsdemux element doesn't support buffer queries, and the GStreamer people don't think it should. Will have to investigate other ways of reporting progress here. Perhaps just using the manifest download result as a sign of progress will be a decent approach.
Created attachment 315936 [details] Patch Definitely a hack, but I don't believe there is any other approach. Modifying the demuxer to handle this case isn't something upstream think is a good idea, in fact they don't think buffering queries should ever work on HLS pipelines. Also note that the percentage value is quite meaningless from the queue's buffer. It's simply used to recognize a difference has been observed and progress of some form is being made.
Comment on attachment 315936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315936&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1198 > + char* name = gst_element_get_name(element); Please use GST_ELEMENT_NAME instead and you won't have to free the name later. Make it const. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1202 > + GstElement* parent = GST_ELEMENT(gst_element_get_parent(element)); gst_element_get_parent is transfer full, you're leaking an element here. Please use GST_ELEMENT_PARENT instead. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1207 > + char* grandpaName = gst_element_get_name(grandparent); When using GST_ELEMENT_NAME, probably you won't need to use this variable, but if you decided to keep it, use complete words. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1220 > +static bool hlsIsProgressing(GstElement* playbin, GstQuery** query) If we are not freeing the query, it can be GstQuery* query. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1223 > + GstIterator* it = gst_bin_iterate_recurse(GST_BIN(playbin)); Please use iterator (complete words). > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1224 > + bool foundHLSElement = gst_iterator_find_custom(it, reinterpret_cast<GCompareFunc>(findHLSElement), &item, NULL); NULL -> nullptr > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1230 > + GstElement* result = static_cast<GstElement*>(g_value_get_object(&item)); Use the GNOME style cast there. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1246 > + if (!hlsIsProgressing(m_pipeline.get(), &query)) Please, add a comment here explaining why we are doing this hack. Please, free the query out of the hlsIsProgressing function, which should be renamed to isHLSProgressing.
Created attachment 315978 [details] Patch Thank you for the detailed review calvaris.
Attachment 315978 [details] did not pass style-queue: ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:806: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:787 and LayoutTests/platform/gtk/TestExpectations:806. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:807: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:788 and LayoutTests/platform/gtk/TestExpectations:807. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3370: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:2234 and LayoutTests/platform/gtk/TestExpectations:3370. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3371: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:2238 and LayoutTests/platform/gtk/TestExpectations:3371. [test/expectations] [5] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315978&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1223 > + GstElement* result = GST_ELEMENT(g_value_get_object(&item)); > + g_value_unset(&item); > + > + return gst_element_query(result, query); You're getting the result, then unset the item and perform the query. When you unset the item, we lose the reference that we own because the unset is going to unref the object. It will be alive after that unset because it is alive inside the pipeline but it will be cleaner if we just do something like: GstElement* resultElement = GST_ELEMENT(g_value_get_object(&item)); bool queryResult = gst_element_query(resultElement, query); g_value_unset(&item); return queryResult;
Created attachment 315984 [details] Patch
Created attachment 315985 [details] Patch
Attachment 315985 [details] did not pass style-queue: ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:806: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:787 and LayoutTests/platform/gtk/TestExpectations:806. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:807: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:788 and LayoutTests/platform/gtk/TestExpectations:807. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3370: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:2234 and LayoutTests/platform/gtk/TestExpectations:3370. [test/expectations] [5] ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3371: Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:2238 and LayoutTests/platform/gtk/TestExpectations:3371. [test/expectations] [5] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315985 [details] Patch Ping me or any other committer for the cq+
(In reply to Build Bot from comment #10) > Attachment 315985 [details] did not pass style-queue: > > > ERROR: > /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations: > 806: Duplicate or ambiguous entry lines > LayoutTests/platform/gtk/TestExpectations:787 and > LayoutTests/platform/gtk/TestExpectations:806. [test/expectations] [5] > ERROR: > /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations: > 807: Duplicate or ambiguous entry lines > LayoutTests/platform/gtk/TestExpectations:788 and > LayoutTests/platform/gtk/TestExpectations:807. [test/expectations] [5] > ERROR: > /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations: > 3370: Duplicate or ambiguous entry lines > LayoutTests/platform/gtk/TestExpectations:2234 and > LayoutTests/platform/gtk/TestExpectations:3370. [test/expectations] [5] > ERROR: > /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations: > 3371: Duplicate or ambiguous entry lines > LayoutTests/platform/gtk/TestExpectations:2238 and > LayoutTests/platform/gtk/TestExpectations:3371. [test/expectations] [5] > Total errors found: 4 in 3 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. Looks like the expectations issues need fixed.
Agreed, but I don't understand why my bug report is getting spammed with them, I don't believe I caused it?
Hm, I guess it's just because you modified the file. We should fix it separately then.
(In reply to Michael Catanzaro from comment #14) > We should fix it separately then. I'll handle this.
> I'll handle this. Sorry, I didn't mean to come across as dismissive of the the issue, I have no problem fixing the linter errors now that I'm aware of them, I was just questioning whether it should block cq+ on this bug. :)
Comment on attachment 315985 [details] Patch Clearing flags on attachment: 315985 Committed r219688: <http://trac.webkit.org/changeset/219688>
All reviewed patches have been landed. Closing bug.
*** Bug 138073 has been marked as a duplicate of this bug. ***