[GStreamer] media/track/in-band/ layout tests introduced in r154908 are failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The media/track/in-band/ layout tests are failing since they were added in r154908.
http://trac.webkit.org/changeset/154908

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=media%2Ftrack%2Fin-band

The VIDEO_TRACK feature seems to be enabled at build-time, but I'm not familiar enough with the feature or tests to investigate at this moment.

[Brendan Long]

It's probably because they need GStreamer 1.1.2 to work correctly, and the test machines don't have that version. Do you want me to just skip these for now?

[Zan Dobersek]

I've already skipped them for the GTK port in r155025.
http://trac.webkit.org/projects/webkit/changeset/155025

They're also failing on EFL though.
http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=media%2Ftrack%2Fin-band

[Brendan Long]

Created attachment 210481 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 210481 [details]
Patch

Clearing flags on attachment: 210481

Committed r155111: <http://trac.webkit.org/changeset/155111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

Reopening since the tests are failing and will continue to fail until we move onto using GStreamer >= 1.1.2.

[Brendan Long]

(In reply to comment #6)
> Reopening since the tests are failing and will continue to fail until we move onto using GStreamer >= 1.1.2.

Did I not skip them correctly, or do you just mean that the fact that they're being skipped means the bug should remain open?

[Zan Dobersek]

(In reply to comment #7)
> (In reply to comment #6)
> > Reopening since the tests are failing and will continue to fail until we move onto using GStreamer >= 1.1.2.
> 
> Did I not skip them correctly, or do you just mean that the fact that they're being skipped means the bug should remain open?

The latter.

[Zan Dobersek]

I reckon that the GStreamer bump will be done at some point in the future, but in a separate bug. Still, until then I'd like to keep one bug entry (i.e. this one) open that notes the reason behind the skipping and what's necessary to get the tests unskipped and passing.

[Carlos Alberto Lopez Perez]

I will change the bug assigned on LayoutTests/platform/gtk/TestExpectations from #103771 to this one for the following tests:

  media/track/track-forced-subtitles-in-band.html
  media/track/track-in-band.html
  media/track/track-in-band-cues-added-once.html
  media/track/track-in-band-style.html
  media/track/track-in-band-legacy-api.html
  media/track/track-in-band-mode.html

I think the above tests are failing for the very same problem discussed on this bug.

[Brendan Long]

(In reply to comment #10)
> I will change the bug assigned on LayoutTests/platform/gtk/TestExpectations from #103771 to this one for the following tests:
> 
>   media/track/track-forced-subtitles-in-band.html
>   media/track/track-in-band.html
>   media/track/track-in-band-cues-added-once.html
>   media/track/track-in-band-style.html
>   media/track/track-in-band-legacy-api.html
>   media/track/track-in-band-mode.html
> 
> I think the above tests are failing for the very same problem discussed on this bug.

Weren't these already skipped? Those tests require MPEG-4 caption support (whatever format that is), and I'm pretty sure GStreamer doesn't support that format.

[Carlos Alberto Lopez Perez]

(In reply to comment #11)
> Weren't these already skipped? Those tests require MPEG-4 caption support (whatever format that is), and I'm pretty sure GStreamer doesn't support that format.

Yes, but they were assigned to bug https://bugs.webkit.org/show_bug.cgi?id=103771 (which seems fixed).

Isn't this the right bug for tracking them? I will open a new bug otherwise

[Brendan Long]

(In reply to comment #12)
> (In reply to comment #11)
> > Weren't these already skipped? Those tests require MPEG-4 caption support (whatever format that is), and I'm pretty sure GStreamer doesn't support that format.
> 
> Yes, but they were assigned to bug https://bugs.webkit.org/show_bug.cgi?id=103771 (which seems fixed).
> 
> Isn't this the right bug for tracking them? I will open a new bug otherwise

I'm not sure. The underlying problem is different. In one case (contents of media/track/in-band/), we can unskip the tests when the build bots have the right version of GStreamer. In the other case (media/track/track-in-band*.html), we need GStreamer to add support for that caption format, then wait for the build bots to get that version.

[Carlos Alberto Lopez Perez]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Weren't these already skipped? Those tests require MPEG-4 caption support (whatever format that is), and I'm pretty sure GStreamer doesn't support that format.
> > 
> > Yes, but they were assigned to bug https://bugs.webkit.org/show_bug.cgi?id=103771 (which seems fixed).
> > 
> > Isn't this the right bug for tracking them? I will open a new bug otherwise
> 
> I'm not sure. The underlying problem is different. In one case (contents of media/track/in-band/), we can unskip the tests when the build bots have the right version of GStreamer. In the other case (media/track/track-in-band*.html), we need GStreamer to add support for that caption format, then wait for the build bots to get that version.

Ok. I have opened a new bug for those tests: https://bugs.webkit.org/show_bug.cgi?id=131546

[Enrique Ocaña]

Created attachment 401154 [details]
Patch

[Philippe Normand]

Comment on attachment 401154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401154&action=review

> Tools/ChangeLog:9
> +        Fixed media/track/in-band tests. Added libkate as a dependency, so gst-plugins-bad detects
> +        it and can build the GstKateDec element.

This won't work on the bots. The SDK doesn't ship libkate currently.

[Enrique Ocaña]

Comment on attachment 401154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401154&action=review

>> Tools/ChangeLog:9
>> +        it and can build the GstKateDec element.
> 
> This won't work on the bots. The SDK doesn't ship libkate currently.

Then I would need some guidance on how to add the dependency to the SDK. This is a topic totally new for me.

> LayoutTests/media/track/in-band/track-in-band-mpegts-metadata.html:-1
> -<!DOCTYPE html>

Note for reviewers:

I've removed this test because it would never succeed with the current tsdemux implementation. That implementation basically skips unknown streams on the mpegts container ( https://github.com/GStreamer/gst-plugins-bad/blob/1.16/gst/mpegtsdemux/tsdemux.c#L1687 ), and those streams are the ones the test relies on in order to get extra pads (exposed as metadata text tracks).

[Enrique Ocaña]

Comment on attachment 401154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401154&action=review

>>> Tools/ChangeLog:9
>>> +        it and can build the GstKateDec element.
>> 
>> This won't work on the bots. The SDK doesn't ship libkate currently.
> 
> Then I would need some guidance on how to add the dependency to the SDK. This is a topic totally new for me.

Libkate support has been added in https://trac.webkit.org/changeset/262941/webkit, but still the test don't passed when using flatpak because avdec_mpeg was missing. Later on, https://trac.webkit.org/changeset/263027/webkit added that missing support, so now all the in-band tests (as modified by this patch) are passing here for me with flatpak.

[Philippe Normand]

Comment on attachment 401154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401154&action=review

> LayoutTests/media/in-band-tracks.js:117
> +        // Sometimes the text tracks take a bit to be found as the media is processed, so we must retry.
> +        if (!inbandTrack1) {
> +            setTimeout(canplaythrough, 100);
> +            return;
> +        }

I'm afraid this could potentially trigger an infinite recursion. Would it make sense to perform a fixed amount and fail the test otherwise?

[Eric Carlson]

Comment on attachment 401154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401154&action=review

>> LayoutTests/media/in-band-tracks.js:117
>> +        }
> 
> I'm afraid this could potentially trigger an infinite recursion. Would it make sense to perform a fixed amount and fail the test otherwise?

I agree that checking for a fixed amount of time before failing the test would be better, as all state is lost when WKTR kills a test for taking too long.

[Enrique Ocaña]

All the tracks are found now at the fist try, so apparently there's no need to retry (setTimeout) anymore. I don't know if this started to happen after having migrated to the flatpak-based development environment or if it was just chance.

On the other hand, now I'm experiencing some flakiness in some of the tests that are requiring some debugging. The next version of the patch will take more than expected.

[Enrique Ocaña]

Created attachment 426908 [details]
Patch

[Enrique Ocaña]

Created attachment 426919 [details]
Patch

[Philippe Normand]

Comment on attachment 426919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426919&action=review

> LayoutTests/ChangeLog:18
> +        * platform/gtk/TestExpectations: Unskip media/track/in-band tests.

Most media expectations are now in the glib TestExpectations. Can you unskip the in-band tests there?

[Enrique Ocaña]

Created attachment 427030 [details]
Patch

[EWS]

Committed r276586 (237022@main): <https://commits.webkit.org/237022@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427030 [details].