Bug 71662 – [GTK] media/event-attributes.html fails

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

It's currently skipped in GTK anyway but here's the diff:

--- /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/layout-test-results/media/event-attributes-expected.txt
+++ /home/phil/gst/jhbuild/build/WebKit/WebKitBuild/Release/layout-test-results/media/event-attributes-actual.txt
@@ -13,14 +13,14 @@
 
 *** changing playback rate
 RUN(video.playbackRate = 2)
+EVENT(volumechange)
+
+*** pausing playback
+RUN(video.pause())
 EVENT(ratechange)
 
 *** setting volume
 RUN(video.volume = 0.5)
-EVENT(volumechange)
-
-*** pausing playback
-RUN(video.pause())
 EVENT(pause)
 
 *** seeking
@@ -32,6 +32,7 @@
 RUN(video.play())
 EVENT(play)
 EVENT(playing)
+EVENT(durationchange)
 EVENT(pause)
 EVENT(ended)

[Simon Pena]

This seems to be failing now only after seeking:

--- /home/spenap/workspace/WebKit/WebKitBuild/Release/layout-test-results/media/event-attributes-expected.txt
+++ /home/spenap/workspace/WebKit/WebKitBuild/Release/layout-test-results/media/event-attributes-actual.txt
@@ -32,6 +32,7 @@
 RUN(video.play())
 EVENT(play)
 EVENT(playing)
+EVENT(durationchange)
 EVENT(pause)
 EVENT(ended)

[Philippe Normand]

Maybe the duration event is wrongly emitted from MediaPlayerPrivateGStreamer::didEnd ?

[Philippe Normand]

(In reply to comment #2)
> Maybe the duration event is wrongly emitted from MediaPlayerPrivateGStreamer::didEnd ?

Might be worth debugging ::durationChanged as well

[Simon Pena]

Cool, I'll take a look into that.

[Simon Pena]

(In reply to comment #4)
> Cool, I'll take a look into that.

OK, there were several things here. First, about my comment #1.
EVENT(durationchange) was indeed emitted from ::didEnd (as you said in
comment #3)

The comment in MediaPlayerPrivateGStreamer::didEnd there explained that
durationChange was emitted in case it was reverse playback. Checking for
m_playbackRate < 0 is enough to avoid wrongly emitting that signal.

But, as soon as I fixed that, I got the other issues in the diff in comment #0.
It turned out that pulseaudio was saving the volume setting of the testrunner
between different test runs.

I will update the gtk.py script so module-stream-restore (the pulseaudio module
responsible for saving and restoring the volume settings) is unloaded for the
test run.

Also, if someone is interested in testing this earlier, it's possible to access
pulseaudio's database:

$tdbtool ~/.pulse/*-stream-volumes.tdb

And remove the offending entry:

tdb> delete sink-input-by-application-name:DumpRenderTree

Then, killing pulseaudio will make it re-load the database, where the setting
for the testrunner won't be found.

[Martin Robinson]

(In reply to comment #5)

> But, as soon as I fixed that, I got the other issues in the diff in comment #0.
> It turned out that pulseaudio was saving the volume setting of the testrunner
> between different test runs.

Wow! Great catch.

[Philippe Normand]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > But, as soon as I fixed that, I got the other issues in the diff in comment #0.
> > It turned out that pulseaudio was saving the volume setting of the testrunner
> > between different test runs.
> 
> Wow! Great catch.

Yes indeed, well done Simón :)

[Simon Pena]

Created attachment 136439 [details]
Patch

[Simon Pena]

(In reply to comment #8)
> Created an attachment (id=136439) [details]
> Patch

I'd be happy to get feedback on the Python part: I don't like my current approach too much, since relying on the _worker_number is a workaround to avoid different workers messing with each other (without that check, a worker could find itself running with the offending PulseAudio module just restored by another worker).

dpranke, I'm CCing you since Phil mentioned you could point out how to improve that.

[Philippe Normand]

(In reply to comment #9)
> (In reply to comment #8)
> > Created an attachment (id=136439) [details] [details]
> > Patch
> 
> I'd be happy to get feedback on the Python part: I don't like my current approach too much, since relying on the _worker_number is a workaround to avoid different workers messing with each other (without that check, a worker could find itself running with the offending PulseAudio module just restored by another worker).
> 

Maybe it'd be better to extend Port's constructor to unload the module and __del__ to unload the module?

[Philippe Normand]

Comment on attachment 136439 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:65
> +            for module in modules_list.split('\n'):

Please use splitlines()

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:70
> +                subprocess.Popen(["pactl", "unload-module", self._module_index], stdout=None, stderr=None)

stdout and stderr are None by default, I think. So no need to set them here.

[Martin Robinson]

Comment on attachment 136439 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:65
>> +        # We want to unload pulseaudio's module-stream-restore, since it
>> +        # remembers volume settings from different runs, and could affect
>> +        # multimedia tests results
>> +        if self._worker_number == 0:
>> +            with open(os.devnull, 'w') as devnull:
>> +                pactl_process = subprocess.Popen(["pactl", "list", "short", "modules"], stdout=subprocess.PIPE, stderr=devnull)
>> +            modules_list = pactl_process.communicate()[0]
>> +            self._module_index = -1
>> +            for module in modules_list.split('\n'):
> 
> Please use splitlines()

It makese sense to split this off into a helper method.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:81
> +        # If pulseaudio's module-stream-restore was previously loaded, we
> +        # restore it here.
> +        if self._worker_number == 0 and self._module_index != -1:
> +            with open(os.devnull, 'w') as devnull:
> +                subprocess.Popen(["pactl", "load-module", "module-stream-restore"], stdout=devnull, stderr=devnull)

Is there a way to disable this for a single process only, so we can avoid putting the system in a bad state if the process crashes?

[Martin Robinson]

(In reply to comment #10)

> Maybe it'd be better to extend Port's constructor to unload the module and __del__ to unload the module?

The constructor sound like a good idea, but doesn't __del__ fire at some random time depending on garbage collection?

[Dirk Pranke]

Comment on attachment 136439 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:60
> +        if self._worker_number == 0:

Why are you doing this change here at all, and why checking if _worker_number == 0? 

If you want this to happen before *any* tests are run (and reset after *all* of them have run), you should do this work in GtkPort.setup_test_run (and it looks like there isn't a corresponding clean_up_test_run(), so we should probably add that and add the call in Manager._clean_up_run in manager.py.

On the other hand, if you want to do this for each individual test, it's not clear to me how this would work when running multiple DRTs simultaneously?

[Simon Pena]

Comment on attachment 136439 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:60
>> +        if self._worker_number == 0:
> 
> Why are you doing this change here at all, and why checking if _worker_number == 0? 
> 
> If you want this to happen before *any* tests are run (and reset after *all* of them have run), you should do this work in GtkPort.setup_test_run (and it looks like there isn't a corresponding clean_up_test_run(), so we should probably add that and add the call in Manager._clean_up_run in manager.py.
> 
> On the other hand, if you want to do this for each individual test, it's not clear to me how this would work when running multiple DRTs simultaneously?

Thanks! I wanted that to happen before tests were run, so setup_test_run and clean_up_test_run seem the way to go. I'll update the patch accordingly.

I'll Also follow Martin and Philippe comments regarding to code.

>>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:65
>>> +            for module in modules_list.split('\n'):
>> 
>> Please use splitlines()
> 
> It makese sense to split this off into a helper method.

I'll fix this in the new version of this patch.

>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:70
>> +                subprocess.Popen(["pactl", "unload-module", self._module_index], stdout=None, stderr=None)
> 
> stdout and stderr are None by default, I think. So no need to set them here.

OK, I'll get that fixed in the next version.

>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:81
>> +                subprocess.Popen(["pactl", "load-module", "module-stream-restore"], stdout=devnull, stderr=devnull)
> 
> Is there a way to disable this for a single process only, so we can avoid putting the system in a bad state if the process crashes?

Dirk comment explained me how to do it, so I'll get it fixed in the next version of the patch.

[Simon Pena]

Created attachment 136654 [details]
Patch

[Philippe Normand]

Comment on attachment 136654 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 136654 [details]
Patch

Clearing flags on attachment: 136654

Committed r113849: <http://trac.webkit.org/changeset/113849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.