mopidy / mopidy-beets

Mopidy extension for playing music from a Beets collection
https://mopidy.com/ext/beets/
MIT License
49 stars 14 forks source link

docs: hide Range header from flask to workaround beets web bug #41

Open mgoltzsche opened 11 months ago

mgoltzsche commented 11 months ago

This is to prevent flask from handling range requests when nginx emulates range requests. Playing large opus files within firefox and gstreamer/mopidy worked for me only after I made this change in my nginx config. I guess this was not a problem previously since older flask versions did not support range queries at all but that seems to have changed partially since flask returns an Accept-Ranges: bytes header. Though, when using flask for streaming directly (without reverse-proxy), audio still doesn't play within firefox or gstreamer. gstreamer logged an error indicating the request is unsupported.

mgoltzsche commented 11 months ago

I just corrected my earlier assumption that streaming wasn't working because of flask 3 usage: Turns out I didn't use flask 3 actually but still 2. Nevertheless, streaming works with gstreamer and firefix only after I made this change.

sumpfralle commented 11 months ago

Thank you for investigating this issue!

Your changes hide all range-related headers from flask : this sounds like a necessary addition to the previous directive (proxy_force_ranges on;). Good!

I feel a bit uncomfortable with adding the comment flask does not fully support range requests without a specific issue reference (in flask). Right now I did not find a range-related issue there. Maybe you could open an issue with a request example (either here or in the flask issue tracker) and reference this issue in your comment? That would be neat!

(otherwise I would just merge your proposal as it is, since it is an obvious improvement, anyway)

mgoltzsche commented 11 months ago

I removed my unconfirmed suspicion from the comment now. Maybe flask supports range requests now but there was another problem in my setup/config? I won't investigate that now since I need an nginx reverse-proxy in front of it anyway now but I may come back to that later at some point eventually.

kingosticks commented 11 months ago

Flask does support range requests and partial responses, but Beets web plugin does not. The plugin ignores any range request and will send the entire file each time (see here). Without any sort of proxy sorting this out (e.g. proxy_force_ranges on), GStreamer will be expecting a 206 Partial Content response code to its range request but be getting a regular 200 response, which indicates the request was not supported.

Given the plugin does not support partial responses, it should not advertise that it does. So it should not be responding with "Accept-Ranges: bytes". However, I can't see where it sets that response header. Do you have a log showing the traffic between GStreamer and Beets?

It's interesting you need the extra nginx config settings, I would have thought proxy_force_ranges on is enough to work around this. And also it's weird that beets cares about the headers you've removed and added. I don't understand that.

mgoltzsche commented 10 months ago

The link you posted shows that

Thus, looking at the code, it seems flask fully supports range requests indeed but the beets web plugin breaks them by overwriting the Content-Length header after it was set by werkzeug, always setting the full file length instead of sending the length of the requested range/chunk that werkzeug has set. Hence, the proper solution might be to remove the line that overwrites the Content-Length header within the beets web plugin. I'll try that...

kingosticks commented 10 months ago

I still can't see where that header processing function is called. But it would make sense that it is called. There was some flask issue showing a wrapped version of sendfile that handled range requests, it was more involved than just not overwriting that header but maybe it was old code. I don't use any of this, i was just curious!

mgoltzsche commented 10 months ago

Removing that line within the beets web plugin really fixed the problem. I created a corresponding beets PR now.

I suggest to merge this PR (since it is a valid workaround currently) and create/merge another one removing the workaround section within the readme as soon as the beet web plugin fix has been released (I anticipate it will be shipped with the beets 1.6.1 release).

mgoltzsche commented 10 months ago

fyi the header processing is part of the response_class which is constructed here. A few lines below, the make_conditional method is called with accept_ranges=True when conditional is True which is the default. That method then calls _process_range_request here which implements the range request handling and setting of those headers.

mgoltzsche commented 10 months ago

I just updated the PR, simplifying the workaround, explaining that it is required prior to beets 1.6.1 only and linking to the PR that fixes the beets web plugin bug. Please re-review!