In RenderThemeGtk::paintMediaPlayButton mediaElement.canPlay() is called. This triggers a position query in the media player. So when the mouse is over the video and the controls are active, the playback position is queried about 30 times/second.
Created attachment 49382 [details] proposed patch
+ if (g_str_equal(GST_MESSAGE_SRC_NAME(message), "pipeline")) + mp->updateStates(); Do a pointer comparison instead Position/Duration queries can fail in the beginning but could return something useful later. So the approach taken here is wrong IMHO. Why do you rename the playbin2 to "pipeline"? :)
(In reply to comment #2) > + if (g_str_equal(GST_MESSAGE_SRC_NAME(message), "pipeline")) > + mp->updateStates(); > > Do a pointer comparison instead > you mean comparing with m_playBin? It can't be accessed from there :/ If found this simpler than passing n_playBin into callback data or implementing an accessor. > Position/Duration queries can fail in the beginning but could return something > useful later. So the approach taken here is wrong IMHO. > I don't cache position ;) For duration it is indeed cached but I made sure that if the query fails at the beginning it will be re-attempted later when playback started. > Why do you rename the playbin2 to "pipeline"? :) Dunno... suggestions? :)
Created attachment 49802 [details] updated patch Rebased patch against master and implemented platformMedia() method so the message callback doesn't need to check message object source name anymore.
Comment on attachment 49802 [details] updated patch 1572 float maxTime = maxTimeSeekable(); 15721573 // FIXME real ranges support 1573 if (!maxTimeSeekable()) 1574 if (!maxTime) 15741575 return TimeRanges::create(); 1575 return TimeRanges::create(minTimeSeekable(), maxTimeSeekable()); 1576 return TimeRanges::create(minTimeSeekable(), maxTime); Unrelated micro-optimization? Is this really a problem? I'd put this in a separate patch, and get Eric Carlson to look at it. 5864 // backend can live at runtime. 5965 typedef struct PlatformMedia { 6066 QTMovie* qtMovie; 67 GstElement* gstPipeline; 6168 } PlatformMedia; This looks weird to me. I don't think our struct should have a QTMovie in it. Maybe we need a #if/#else here, instead? Or event better, just write an implementation for platformMedia that returns the playbin! See: 77 virtual PlatformMedia platformMedia() const { return NoPlatformMedia; } 676 return paintMediaButton(paintInfo.context, r, mediaElement->canPlay() ? m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize); 676 return paintMediaButton(paintInfo.context, r, mediaElement->paused() ? m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize); This seems totally unrelated?
(In reply to comment #5) > (From update of attachment 49802 [details]) > 1572 float maxTime = maxTimeSeekable(); > 15721573 // FIXME real ranges support > 1573 if (!maxTimeSeekable()) > 1574 if (!maxTime) > 15741575 return TimeRanges::create(); > 1575 return TimeRanges::create(minTimeSeekable(), maxTimeSeekable()); > 1576 return TimeRanges::create(minTimeSeekable(), maxTime); > > Unrelated micro-optimization? Is this really a problem? I'd put this in a > separate patch, and get Eric Carlson to look at it. > I can remove it, no worries. > 5864 // backend can live at runtime. > 5965 typedef struct PlatformMedia { > 6066 QTMovie* qtMovie; > 67 GstElement* gstPipeline; > 6168 } PlatformMedia; > > This looks weird to me. I don't think our struct should have a QTMovie in it. > Maybe we need a #if/#else here, instead? Or event better, just write an > implementation for platformMedia that returns the playbin! See: > The comment above the declaration: // Structure that will hold every native // types supported by the current media player. // We have to do that has multiple media players // backend can live at runtime. So as I understood it we need to stuff inside this structure. Or should we instead conditionally define it depending on the platform? Now I am confused! :) > 77 virtual PlatformMedia platformMedia() const { return NoPlatformMedia; > } > > 676 return paintMediaButton(paintInfo.context, r, mediaElement->canPlay() > ? m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize); > 676 return paintMediaButton(paintInfo.context, r, mediaElement->paused() ? > m_playButton.get() : m_pauseButton.get(), m_panelColor, m_mediaIconSize); > > This seems totally unrelated? It is not! :) canPlay() triggers a call to endedPlayback() which queries the duration. This query is useless, just checking paused() is enough in this context and this what MediaControlPlayButtonElement::updateDisplayType does, FWIW.
(In reply to comment #6) > // Structure that will hold every native > // types supported by the current media player. > // We have to do that has multiple media players > // backend can live at runtime. > > So as I understood it we need to stuff inside this structure. Or should we > instead conditionally define it depending on the platform? Now I am confused! > :) Made sense to me after reading a bit more of the code. No, I think your original patch is right. This is so that, say, if a port is able to use both the Mac Media Player, and the GStreamer one, you'll have both available, and choose which to use in run time. > > This seems totally unrelated? > > It is not! :) canPlay() triggers a call to endedPlayback() which queries the > duration. This query is useless, just checking paused() is enough in this > context and this what MediaControlPlayButtonElement::updateDisplayType does, > FWIW. Hah. OK, then. I wonder if it makes sense as a separate change, though?
(In reply to comment #7) > (In reply to comment #6) > > // Structure that will hold every native > > // types supported by the current media player. > > // We have to do that has multiple media players > > // backend can live at runtime. > > > > So as I understood it we need to stuff inside this structure. Or should we > > instead conditionally define it depending on the platform? Now I am confused! > > :) > > Made sense to me after reading a bit more of the code. No, I think your > original patch is right. This is so that, say, if a port is able to use both > the Mac Media Player, and the GStreamer one, you'll have both available, and > choose which to use in run time. > Actually I don't agree. The current member name was poorly chosen and should change. The value is really media engine specific, and I think it would make more sense to have a generic type that is cast to the specific type when it is used. If a port is able to use both a QuickTime and GStreamer backend I would expect it to have a different media engine for each, and obviously only one will be used for a single movie.
(In reply to comment #7) > > > > This seems totally unrelated? > > > > It is not! :) canPlay() triggers a call to endedPlayback() which queries the > > duration. This query is useless, just checking paused() is enough in this > > context and this what MediaControlPlayButtonElement::updateDisplayType does, > > FWIW. > > Hah. OK, then. I wonder if it makes sense as a separate change, though? Please re-read the bug description :) This change is the main reason for this bug.
(In reply to comment #8) > Actually I don't agree. The current member name was poorly chosen and should > change. The value is really media engine specific, and I think it would make > more sense to have a generic type that is cast to the specific type when it is > used. > > If a port is able to use both a QuickTime and GStreamer backend I would expect > it to have a different media engine for each, and obviously only one will be > used for a single movie. Right, but how will it decide which one to use in runtime, then?
(In reply to comment #9) > (In reply to comment #7) > > > > > > > This seems totally unrelated? > > > > > > It is not! :) canPlay() triggers a call to endedPlayback() which queries the > > > duration. This query is useless, just checking paused() is enough in this > > > context and this what MediaControlPlayButtonElement::updateDisplayType does, > > > FWIW. > > > > Hah. OK, then. I wonder if it makes sense as a separate change, though? > > Please re-read the bug description :) This change is the main reason for this > bug. I know, but AFAIK we can commit this change first, along with the media duration changes, and then on a second commit add the check to only handle messages coming from playbin, right?
(In reply to comment #10) > (In reply to comment #8) > > Actually I don't agree. The current member name was poorly chosen and should > > change. The value is really media engine specific, and I think it would make > > more sense to have a generic type that is cast to the specific type when it is > > used. > > > > If a port is able to use both a QuickTime and GStreamer backend I would expect > > it to have a different media engine for each, and obviously only one will be > > used for a single movie. > > Right, but how will it decide which one to use in runtime, then? Register a media engine for both backends and *you* will get to decide based on the content type. chooseBestEngineForTypeAndCodecs() in MediaPlayer.cpp always iterates through every registered media engine when it is asked to open a movie, asking each one if it supports the movie's (extended) MIME type, and uses the one that claims to have the best support.
Created attachment 50698 [details] updated patch
(In reply to comment #13) > Created an attachment (id=50698) [details] > updated patch This patch doesn't contain the platformMedia stuff, it will be added in a patch for Bug 36112. So hopefully we can now get this reduced patch in? :)
Comment on attachment 50698 [details] updated patch You're still adding too much to one single change. You're changing three separate things that are easily separatable: the decision on how to paint the button, the changes to media duration query, and the filtering of the messages. I understand they are all part of fixing this bug, but this does not mean they should go in at the same time. Having all of them in at the same time will make this commit semi-unuseful for bisecting. What caused a problem with the play/pause behavior changing, if it does? If we find this commit to be the problem, we'll have to manually check which of the three changes is responsible. So, I'm OK with all three changes, but let's split them, and then we'll get them in, yeah =).
Created attachment 50703 [details] micro patched :)
Comment on attachment 50703 [details] micro patched :) Thanks!
Landed in r56006
This makes videos not go out of 'playing' when they end, UI-wise. I assume the real problem is we are not marking the video 'paused' attribute to true correctly, but before we fix this, using that attribute to make UI decisions sounds unwise. Let's make this testable, fix the root cause, and then we can reland the optimization.
Created attachment 50789 [details] a possible layout test for the root cause The spec is not very specific on whether video.paused should be true when the video ends, but I think it should be if we're not looping, from my reading, since I consider that to be a case of 'paused for user interaction' - the user needs to take action to get the video playing again, or to seek it. If it is not, then we need to improve this optimization to also take the ended attribute into consideration. I made this test based on media/video-loop.html, and we currently fail it. Eric, can you check if Mac passes this test? Your help would be appreciated in figuring this behaviour out =).
The spec also mentions: It is possible for a media element to have both ended playback and paused for user interaction at the same time. Our player pauses itself at the end of playback but the media element doesn't know about it because its pause() method doesn't query the player. So I think we could have it. It would require the HTMLMediaElement::paused() to proxy to MediaPlayer::paused(), when a player is available. If not the m_paused attribute could be used as fallback. What do you think?
(In reply to comment #21) > The spec also mentions: > > It is possible for a media element to have both ended playback and paused for user interaction at the > same time. > > Our player pauses itself at the end of playback but the media element doesn't know about it because its > pause() method doesn't query the player. > > So I think we could have it. It would require the HTMLMediaElement::paused() to proxy to > MediaPlayer::paused(), when a player is available. If not the m_paused attribute could be used as > fallback. What do you think? I think it is definitely worth a try. Hopefully it won't require significant changes to layout tests, if it does I fear that it will break existing content, but lets see.
(In reply to comment #22) > (In reply to comment #21) > > So I think we could have it. It would require the HTMLMediaElement::paused() to proxy to > > MediaPlayer::paused(), when a player is available. If not the m_paused attribute could be used as > > fallback. What do you think? > > I think it is definitely worth a try. Hopefully it won't require significant changes to layout tests, if it does I fear that it will break existing content, but lets see. Tried this, the patch breaks 7 media tests in the GTK+ port. So I think we should leave the ::paused() method as it is now. Eric, what do you think about Gustavo's proposed test in comment 20 ?
(In reply to comment #20) > Created an attachment (id=50789) [details] > a possible layout test for the root cause > > The spec is not very specific on whether video.paused should be true when the video ends, but I think it should be if we're not looping, from my reading, since I consider that to be a case of 'paused for user interaction' - the user needs to take action to get the video playing again, or to seek it. If it is not, then we need to improve this optimization to also take the ended attribute into consideration. I made this test based on media/video-loop.html, and we currently fail it. Eric, can you check if Mac passes this test? Your help would be appreciated in figuring this behaviour out =). The test fails on Mac too. At the end of playback ended is true and paused is false.
Created attachment 61367 [details] updated patch
Comment on attachment 61367 [details] updated patch > + Node* node = o->node(); > + if (!node) > + return false; > + > + MediaControlPlayButtonElement* btn = static_cast<MediaControlPlayButtonElement*>(node); > + if (!btn) > return false; > Is it possible for node to be non-NULL and the static cast to return NULL? IOW, is the second test necessary? r=me
(In reply to comment #26) > (From update of attachment 61367 [details]) > > + Node* node = o->node(); > > + if (!node) > > + return false; > > + > > + MediaControlPlayButtonElement* btn = static_cast<MediaControlPlayButtonElement*>(node); > > + if (!btn) > > return false; > > > Is it possible for node to be non-NULL and the static cast to return NULL? IOW, is the second test necessary? > > r=me Right the second test is overkill , sorry ;)
Created attachment 61386 [details] updated patch
Landed in http://trac.webkit.org/changeset/63214 Thanks for the review!