[webkit-reviews] review requested: [Bug 117354] [GStreamer] Don't set state to NULL until element is destroyed : [Attachment 210138] Patch

[bugzilla-daemon at webkit.org]

Andre Moreira Magalhaes <andrunko at gmail.com> has asked	for review:
Bug 117354: [GStreamer] Don't set state to NULL until element is destroyed
https://bugs.webkit.org/show_bug.cgi?id=117354

Attachment 210138: Patch
https://bugs.webkit.org/attachment.cgi?id=210138&action=review

------- Additional Comments from Andre Moreira Magalhaes <andrunko at gmail.com>
(In reply to comment #4)
> > For this patch here, something important is indeed missing. After going to
READY, e.g. audio devices are still opened. So there should be a timeout, after
which the element is set to NULL.
> 
> Yes, that sounds like a good idea to me.

Updated patch should handle this. I used a 1 minute timeout for now.

I found an issue when setting the pipeline to READY on didEnd() where the
updateStates() method was being invoked and resetting the network/ready states
thus making the HTMLMediaElement recreate the player when playing the video
again after EOS. When setting to NULL on EOS the updateStates() method is not
called (pipeline flushing), hence why we didn't have this issue before.
To fix it the patch added a check for EOS on updateStates before updating the
network/ready states.

So what happens now is that when finishing playing a video (EOS) we set the
pipeline to READY state and if the video is played again (without reloading
page of course) before the 1 minute timeout we go from READY->PAUSED->PLAYING
instead of starting from NULL, thus saving some resources loading (e.g.
metadata).

Apart from the possible (if played before timeout) gain when replaying videos
on EOS I don't see any other advantage in not setting the pipeline to NULL as
it is done today.

Please let me know what you think.