Bug 88691 – [BlackBerry] Fix two problems introduced by the userDrivenSeekTimer in MediaPlayerPrivate

• Component: WebKit BlackBerry
• Status: CLOSED INVALID

[Max Feil]

The main fix that my patch covers is for short media. The m_userDrivenSeekTimer is causing unwanted repeats of short media such as sound effects because it is causing the current time to not reflect that the media has finished playing. This problem only affects media whose duration is close to or less than the SeekSubmissionDelay, which is currently set to 100ms. My fix is to ignore the userDrivenSeekTimer in MediaPlayerPrivate::currentTime() if the duration of the media is within twice the SeekSubmissionDelay. Seek drag smoothness is a non-issue for such short media.

I discovered this problem on the BrickBreakerRevolution game. The sound of the ball hitting the paddle or bricks would  repeat. I have written an automated test using a sound effect from the game.

A second less serious problem is the way the m_userDrivenSeekTimer itself is implemented. When MediaPlayerPrivate::seek() is called, there is always a 100ms delay even if the timer is not running! The timer is supposed to space out multiple seeks, but currently it is slowing down even single seeks. I fixed this in my patch by improving the way the timer fired function is called.

[Max Feil]

Created attachment 146656 [details]
Patch

[Max Feil]

(In reply to comment #0)
> ...there is always a 100ms delay even if the timer is not running...

I have not written a test for this part of the fix because a 100ms timing difference cannot be reliably tested, in my opinion.

[Antonio Gomes]

Comment on attachment 146656 [details]
Patch

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

Code looks ok, I have some questions, and one request: split it up in two patches/bugs, since we are dealing with two unrelated bugs here.

> Source/WebCore/ChangeLog:19
> +        is a non-issue for such short media.
> +
> +
> +        Test: platform/blackberry/media/short-media-repeats-correctly.html

nit: extra line here

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:237
> +    return m_userDrivenSeekTimer.isActive() && !shortMedia ? m_lastSeekTime: m_platformPlayer->currentTime();

nit: needs a space before :

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:254
> +    if (m_lastSeekTimePending) {
> +        m_platformPlayer->seek(m_lastSeekTime);
> +        m_lastSeekTimePending = false;
> +        m_userDrivenSeekTimer.startOneShot(SeekSubmissionDelay);
> +    }

could you explain why m_lastSeekTimePending is needed? I take it that it is because you are restarting the timer from within line 253, but why do we need it?

Also Max, I think we should do two patches for these two issues.

[Eric Carlson]

Comment on attachment 146656 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:236
> +    bool shortMedia = m_platformPlayer->duration() < SeekSubmissionDelay * 2.0;

Nit: you have a const for SeekSubmissionDelay, why not use another one for "very short duration"?

[Max Feil]

(In reply to comment #3)
> > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:237
> > +    return m_userDrivenSeekTimer.isActive() && !shortMedia ? m_lastSeekTime: m_platformPlayer->currentTime();
> 
> nit: needs a space before :

Note that this part is YOUR code that I left untouched. Are you saying you don't like your own code? ;)

[Max Feil]

(In reply to comment #3)
> could you explain why m_lastSeekTimePending is needed? I take it that it is because you are restarting the timer from within line 253, but why do we need it?

This flag is needed so that userDrivenSeekTimerFired() knows whether or not to perform the seek. The only case where this flag will not be set is if no MediaPlayerPrivate::seek() call came in while the timer was active, in which case it's important to do nothing. I could encode this flag's information into the m_lastSeekTime float, for example by initializing it and resetting it to NAN and using isnan(). But I feel that using a separate bool is a more portable approach. Let me know if you would like me to get rid of m_lastSeekTimePending and use m_lastSeekTime values of NAN instead.

> Also Max, I think we should do two patches for these two issues.

I will split this bug into two...

[Antonio Gomes]

(In reply to comment #5)
> (In reply to comment #3)
> > > Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:237
> > > +    return m_userDrivenSeekTimer.isActive() && !shortMedia ? m_lastSeekTime: m_platformPlayer->currentTime();
> > 
> > nit: needs a space before :
> 
> Note that this part is YOUR code that I left untouched. Are you saying you don't like your own code? ;)

I blame the reviewer then :p

> This flag is needed so that userDrivenSeekTimerFired() knows whether or not to perform the seek. The only case where this flag will not be set is if no MediaPlayerPrivate::seek() call came in while the timer was active, in which case it's important to do nothing. I could encode this flag's information into the m_lastSeekTime float, for example by initializing it and resetting it to NAN and using isnan(). But I feel that using a separate bool is a more portable approach. Let me know if you would like me to get rid of m_lastSeekTimePending and use m_lastSeekTime values of NAN instead.

I thought of reusing m_lastSeekTime as well, but maybe that would not be so clean. I am ok with either ways, lets just add this info to the change log as well..


> > Also Max, I think we should do two patches for these two issues.
> 
> I will split this bug into two...

Thanks.

[Max Feil]

Split into bug 88733 and bug 88732.

[Max Feil]

Closing - being tracked under other bugs. Patches coming soon.