Bug 141469 – [GTK] Layout Test http/tests/media/hls/hls-progress.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Marcos Chavarría Teijeiro (irc: chavaone)]

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

[Charlie Turner]

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.

[Charlie Turner]

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.

[Charlie Turner]

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.

[Xabier Rodríguez Calvar]

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.

[Charlie Turner]

Created attachment 315978 [details]
Patch

Thank you for the detailed review calvaris.

[Build Bot]

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.

[Xabier Rodríguez Calvar]

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;

[Charlie Turner]

Created attachment 315984 [details]
Patch

[Charlie Turner]

Created attachment 315985 [details]
Patch

[Build Bot]

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.

[Xabier Rodríguez Calvar]

Comment on attachment 315985 [details]
Patch

Ping me or any other committer for the cq+

[Michael Catanzaro]

(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.

[Charlie Turner]

Agreed, but I don't understand why my bug report is getting spammed with them, I don't believe I caused it?

[Michael Catanzaro]

Hm, I guess it's just because you modified the file. We should fix it separately then.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> We should fix it separately then.

I'll handle this.

[Charlie Turner]

> 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. :)

[WebKit Commit Bot]

Comment on attachment 315985 [details]
Patch

Clearing flags on attachment: 315985

Committed r219688: <http://trac.webkit.org/changeset/219688>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

*** Bug 138073 has been marked as a duplicate of this bug. ***