[webkit-reviews] review requested: [Bug 85994] [GStreamer] Source element fails with .ogg files : [Attachment 212174] Patch to handle 200 response on HTTP range requests

[bugzilla-daemon at webkit.org]

Andre Moreira Magalhaes <andrunko at gmail.com> has asked	for review:
Bug 85994: [GStreamer] Source element fails with .ogg files
https://bugs.webkit.org/show_bug.cgi?id=85994

Attachment 212174: Patch to handle 200 response on HTTP range requests
https://bugs.webkit.org/attachment.cgi?id=212174&action=review

------- Additional Comments from Andre Moreira Magalhaes <andrunko at gmail.com>
This patch should fix the issue with servers returning 200 on range requests
(instead of 206).

But but, even with this patch the webpage http://playmapscube.com won't load.
What happens is that this page tries to load 44 audio files, which creates 44
pipelines (and sinks) and the pipelines are put on PAUSED state (autoload). The
problem is that pulsesink (used by autoaudiosink here) will create pulse
streams on READY state and pulse has a limit on allowed streams (32 here on PA
3.0) and pulsesink will fail with "not-negotiated" when hitting this limit,
thus failing the pipeline and preventing the page from loading.

I investigated the issue but there doesn't seem to be an easy solution here. To
fix this we have the following options:
1 - Change pulsesink to only create pulse streams on PLAYING (and possible
release them on EOS) - requires upstream blessing, I asked upstream what they
think (awaiting response) but I am not that confident we can make this change
2 - Change pulse to allow setting this limit on runtime and add a property on
pulsesink to set this property. We could then update this property on the
webkit player if we ever hit the limit - not ideal also, requires changing on
pulse/gst/webkit
3 - Increase the limit on pulse (already planned by upstream for PA 5.0
according to Ford_Perfect). This could solve the issue with this specific page
but the general issue would still be there and we could still hit it even with
a higher limit.
4 - Use a custom audiosink that would mix all audios into one output and only
have one connection to pulse (or any other sink). This would be ideal and would
fix the issue once and for all but it could bring its own issues and it would
require a good amount of non-trivial work. We would have to deal with the
pipelines separately (e.g. pause) even though they would share the same sink,
we would also have to make sure videos are in sync with audios accordingly,
etc. So I wouldn't go this route unless we can't find other option.
5 - Change webkit to use fakesink for all audio sinks until we actually start
the playback, where we would update the pipeline sink to use autoaudiosink.
This also has its own set of issues. When the pipeline is on PAUSED the sink
already contains the first buffer and changing sinks would at least loose this
first buffer. Also changing sinks requires the pipeline to be on NULL state
(which resets the pipeline and all cached data) or we could use padblocks but
this is also not 100% reliable.
6 - *HACK* - set a limit of streams on webkit and use fakesink for all new
audio sinks when hitting this limit. No further comments on this "solution" :P

Any other idea?

To test that the attached patch works just update
MediaPlayerPrivateGStreamer.cpp to use fakesink instead of autoaudiosink on
createAudioSink() and try loading playmapscube (it should work).

I am probably not working on this issue anymore (apart from review requests on
this patch), but would gladly discuss the possible solutions.