Bug 83874 - [GTK] r114021 triggered media flakyness
Summary: [GTK] r114021 triggered media flakyness
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on: 84254
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 03:05 PDT by Philippe Normand
Modified: 2012-04-19 02:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.69 KB, patch)
2012-04-18 07:58 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2012-04-18 15:35 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-04-13 03:05:24 PDT
At least on GTK skipping:

media/video-load-require-user-gesture.html 
media/video-play-require-user-gesture.html

works around the issue but a proper solution is required. I won't have time to work on it this week though. Skipping those two for now.
Comment 1 Philippe Normand 2012-04-13 03:13:13 PDT
The flaky tests list:

Unexpected flakiness: text diff mismatch (25)
  fast/forms/text-input-event.html = TEXT PASS
  media/track/track-mode.html = TEXT PASS
  media/video-loop.html = TEXT PASS
  media/video-muted.html = TEXT PASS
  media/video-pause-immediately.html = TEXT PASS
  media/video-play-empty-events.html = TEXT PASS
  media/video-play-pause-events.html = TEXT PASS
  media/video-play-pause-exception.html = TEXT PASS
  media/video-playbackrate.html = TEXT PASS
  media/video-played-collapse.html = TEXT PASS
  media/video-played-ranges-1.html = TEXT PASS
  media/video-played-reset.html = TEXT PASS
  media/video-preload.html = TEXT PASS
  media/video-reverse-play-duration.html = TEXT PASS
  media/video-seek-past-end-paused.html = TEXT PASS
  media/video-seek-past-end-playing.html = TEXT PASS
  media/video-set-rate-from-pause.html = TEXT PASS
  media/video-source-load.html = TEXT PASS
  media/video-src-invalid-poster.html = TEXT PASS
  media/video-src-none.html = TEXT PASS
  media/video-timeupdate-during-playback.html = TEXT PASS
  media/video-timeupdate-reverse-play.html = TEXT PASS
  media/video-transformed.html = TEXT PASS
  media/video-volume.html = TEXT PASS
  media/video-zoom-controls.html = TEXT PASS
Comment 2 Simon Pena 2012-04-18 02:00:50 PDT
Running the tests with --run-singly (so each test uses its 
own DumpRenderTree) works fine. That reinforces the idea that the
MediaPlaybackRequiresUserGesture property, set on the following tests:

media/video-load-require-user-gesture.html 
media/video-play-require-user-gesture.html

is affecting the others.
Comment 3 Simon Pena 2012-04-18 07:58:33 PDT
Created attachment 137687 [details]
Patch
Comment 4 Martin Robinson 2012-04-18 08:07:17 PDT
Comment on attachment 137687 [details]
Patch

The API looks good to me. We just need one more GTK+ reviwer to approve the API and give the official r+.
Comment 5 Philippe Normand 2012-04-18 08:12:40 PDT
Comment on attachment 137687 [details]
Patch

Thanks!
Comment 6 WebKit Review Bot 2012-04-18 08:44:33 PDT
Comment on attachment 137687 [details]
Patch

Clearing flags on attachment: 137687

Committed r114506: <http://trac.webkit.org/changeset/114506>
Comment 7 WebKit Review Bot 2012-04-18 08:44:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Eric Carlson 2012-04-18 11:04:16 PDT
Comment on attachment 137687 [details]
Patch

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

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430
> +                 "media-playback-requires-user-gesture", FALSE,

This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. 

Does the problem reproduce without this?
Comment 9 Philippe Normand 2012-04-18 11:31:47 PDT
Reopening because the patch was rolled out.
Comment 10 Philippe Normand 2012-04-18 15:27:55 PDT
(In reply to comment #8)
> (From update of attachment 137687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review
> 
> > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430
> > +                 "media-playback-requires-user-gesture", FALSE,
> 
> This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. 
> 

Right. 

> Does the problem reproduce without this?

No, but I don't think it's an issue to keep this line anyway.

I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :)

Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException);

And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false);

Fixing the tests fixes flakyness here. But I think we still need Simon's patch anyway, maybe in a new bugzilla entry Simon?
Comment 11 Philippe Normand 2012-04-18 15:35:30 PDT
Created attachment 137788 [details]
Patch
Comment 12 Eric Carlson 2012-04-18 17:06:20 PDT
(In reply to comment #10)
> 
> I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :)
> 
> Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException);
> 
> And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false);
> 
  Oops :-(
Comment 13 Eric Carlson 2012-04-18 17:07:23 PDT
Comment on attachment 137788 [details]
Patch

Thanks for fixing my mistake!
Comment 14 Philippe Normand 2012-04-18 17:12:03 PDT
Comment on attachment 137788 [details]
Patch

Clearing flags on attachment: 137788

Committed r114586: <http://trac.webkit.org/changeset/114586>
Comment 15 Philippe Normand 2012-04-18 17:12:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Simon Pena 2012-04-19 02:38:35 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 137687 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review
> > 
> > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430
> > > +                 "media-playback-requires-user-gesture", FALSE,
> > 
> > This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. 
> > 
> 
> Right. 
> 
> > Does the problem reproduce without this?
> 
> No, but I don't think it's an issue to keep this line anyway.

When I added that line to the DumpRenderTree, I was assuming that, under some
concurrency situations, some tests were run with that setting enabled. Since
they passed fine when I used the --run-singly option, I though I was right.

However, after Phil's findings:

> I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :)
> 
> Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException);
> 
> And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false);

This does seem the cause for flakyness, and not the other.
 
> Fixing the tests fixes flakyness here. But I think we still need Simon's patch anyway, maybe in a new bugzilla entry Simon?

Yes, I guess we should still provide the API to enable/disable the
mediaPlaybackRequiresUserGesture setting (it is provided in other ports),
but we no longer need to clean up that setting from the DumpRenderTree.

I'll file a new bug for that, and attaching this patch without the DumpRenderTree part.

Thanks for the investigation, and sorry for the trouble!