Bug 57673 – Media Element's muted value ignored if set before play

• Component: Media
• Status: RESOLVED FIXED

[David Humphrey (humphd)]

Here's a testcase where the value of muted gets reset when the media resource actually starts playing.  The element does get muted, according to the controls for the element, but that value isn't being passed down to, or is being ignored by, the player.

[David Humphrey (humphd)]

I'm testing this on Chrome/Chromium, and Paul Irish is telling me that this might not belong in WebKit proper.  Let me know if I need to refile elsewhere.

[David Humphrey (humphd)]

See also https://code.google.com/p/chromium/issues/detail?id=70777

[Jer Noble]

Testcase does not reproduce in WebKit/Safari, as only a .ogv source is provided.

[David Humphrey (humphd)]

http://scotland.proximity.on.ca/dxr/tmp/mute-mp3.html

Also fails, as described above.

[Jer Noble]

Verified that this also reproduces on WebKit/Safari.

[David Humphrey (humphd)]

Created attachment 88351 [details]
Possible fix

Here's a possible fix.  This change fixes the case I linked above.  I couldn't figure out where the tests for the media element were, and no one on irc was around to answer.  If you can guide me, I'll do a test for this, too.

[Jer Noble]

David, the media layout tests are in LayoutTests/media/.  Writing a test may be difficult, because HTMLMediaElement::isMuted() will return true, even if the MediaPlayer is still generating sound.

[David Humphrey (humphd)]

Created attachment 88364 [details]
Test for mute before play.

Here's a test.

[WebKit Review Bot]

Attachment 88364 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

The test case passes on my install of Safari, which does not have your fix.  I don't believe the test case tests the failure condition correctly.

(This is what I mean by it being a hard test case to write.) :)

[David Humphrey (humphd)]

Comment on attachment 88364 [details]
Test for mute before play.

mmm, clearly I shouldn't be writing tests at 1 am :)  Yeah, I agree that this is untestable from content scripts.

[Eric Carlson]

(In reply to comment #11)
> (From update of attachment 88364 [details])
> mmm, clearly I shouldn't be writing tests at 1 am :)  Yeah, I agree that this is untestable from content scripts.

It should be possible to create a manual test in Source/WebCore/manual-tests/. I think you want to have a test that sets muted to true before setting src, and verify that no sound is emitted when the movie plays.

[Eric Carlson]

(In reply to comment #12)
> 
> It should be possible to create a manual test in Source/WebCore/manual-tests/. I think you want to have a test that sets muted to true before setting src, and verify that no sound is emitted when the movie plays.

Oh, and the test should be part of the patch instead of a separate attachment.

[David Humphrey (humphd)]

OK, I'll do that.  Tell me, what's the difference here:

./third_party/WebKit/Source/WebCore/manual-tests
./third_party/WebKit/WebCore/manual-tests

[Eric Carlson]

(In reply to comment #14)
> OK, I'll do that.  Tell me, what's the difference here:
> 
> ./third_party/WebKit/Source/WebCore/manual-tests
> ./third_party/WebKit/WebCore/manual-tests

WebKit/WebCore/ doesn't exist any more, you probably had a modified file in there somewhere when the repository was updated.

[David Humphrey (humphd)]

Created attachment 88547 [details]
Fix + manual test

Here's the fix with a manual test.  I was unable to find any test media in the tree that also has sound, so I've followed the example of some other manual tests and linked to urls in svn.  If there is a more suitable media resource, please let me know.  Once again, irc is getting me nowhere.

[Eric Carlson]

Comment on attachment 88547 [details]
Fix + manual test

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

"bugzilla-57673.html" won't be a terribly helpful name to someone browsing the test directory later, maybe something like "media-muted.html" to follow the pattern of several of the existing manual media tests?

This patch is missing the ChangeLog. If you aren't sure how to generate it, http://www.webkit.org/coding/contributing.html has a good description. 

Also if you set the cq? flag when you set r?, the reviewer will set cq+ when your patch is r+'ed and the commit bot will commit it automatically.

Marking r- because of the missing ChangeLog, but please also consider the load() comment and test name suggestion.

> third_party/WebKit/Source/WebCore/manual-tests/bugzilla-57673.html:22
> +            vid.src = "http://src.chromium.org/svn/trunk/src/chrome/test/data/media/" + findMediaFile("video", "bear");
> +
> +            vid.load();

vid.load() is unnecessary, setting vid.src triggers the resource selection algorithm.

[Eric Carlson]

(In reply to comment #16)
>  Once again, irc is getting me nowhere.

I (eric_carlson) am usually on #webkit from early morning through early afternoon Pacific time.

[David Humphrey (humphd)]

Created attachment 88754 [details]
Fix + manual test + review fixes

Thanks for the review, Eric.  Here's a new patch with the fixes you suggest, plus a ChangeLog entry.  Hopefully I've done this the way you want.

[David Humphrey (humphd)]

Created attachment 88757 [details]
Fix + manual test + review fixes (whitespace fix)

That patch had some white space issues in the ChangeLog, here's a better one.

[Eric Carlson]

Comment on attachment 88757 [details]
Fix + manual test + review fixes (whitespace fix)

Thanks David!

[WebKit Commit Bot]

Comment on attachment 88757 [details]
Fix + manual test + review fixes (whitespace fix)

Rejecting attachment 88757 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
==========================================
|--- third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp	(revision 82928)
|+++ third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file third_party/WebKit/Source/WebCore/manual-tests/media-muted.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Carlson', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/8348803

[Mihai Parparita]

(In reply to comment #22)
> Rejecting attachment 88757 [details] from commit-queue.
> Last 500 characters of output:
> ==========================================
> |--- third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp    (revision 82928)
> |+++ third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp    (working copy)
> --------------------------
> No file to patch.  Skipping patch.

It looks like the patch did not apply cleanly because the paths are prefixed with third_party/WebKit, which i the location of WebKit inside the Chromium tree. That prefix should be absent for patches on the WebKit side.

[Mihai Parparita]

(In reply to comment #23)
> It looks like the patch did not apply cleanly because the paths are prefixed with third_party/WebKit, which i the location of WebKit inside the Chromium tree. That prefix should be absent for patches on the WebKit side.

More generally, see http://dev.chromium.org/developers/contributing-to-webkit and the pointer towards using webkit-patch upload, which will generally do The Right Thing as far as patch paths, ChangeLogs, etc.

[David Humphrey (humphd)]

Created attachment 88908 [details]
Fix + manual test + review fixes (whitespace fix, webkit)

Switched to a WebKit tree.

[Eric Carlson]

Comment on attachment 88908 [details]
Fix + manual test + review fixes (whitespace fix, webkit)

Hopefully the third time will be a charm :-)

[WebKit Commit Bot]

Comment on attachment 88908 [details]
Fix + manual test + review fixes (whitespace fix, webkit)

Clearing flags on attachment: 88908

Committed r83373: <http://trac.webkit.org/changeset/83373>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.