Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
After looking at it, here's the list of optional headers we might be interested in : `Access-Control-Allow-Origin` Indicates whether a resource can be shared, via returning the literal value of the Origin request header (which can be `null`) or `*` in a response. `Access-Control-Allow-Credentials` Indicates whether a resource can be shared when request's credentials mode is include. For a CORS preflight request, request's credentials mode is always omit, but for any subsequent CORS requests it might not be. Support therefore needs to be indicated as part of the HTTP response to the CORS preflight request as well. An HTTP response to a CORS preflight request can include the following headers: `Access-Control-Allow-Methods` Indicates which methods are supported by the resource for the purposes of the CORS protocol. The `Allow` header is not relevant for the purposes of the CORS protocol. `Access-Control-Allow-Headers` Indicates which headers are supported by the resource for the purposes of the CORS protocol. `Access-Control-Max-Age` Indicates how long the information provided by the `Access-Control-Allow-Methods` and `Access-Control-Allow-Headers` headers can be cached. An HTTP response to a CORS request that is not a CORS preflight request can also include the following header: `Access-Control-Expose-Headers` Indicates which headers can be exposed as part of the HTTP response, via listing their names. Should we make an option per header (sounds plain silly to me), or keep the current code that let the user says which header(s) she wants ? Cheers, Cyril 2014-12-04 6:57 GMT+01:00 Max Kellermann : > On 2014/12/03 23:09, X Ryl wrote: > > Actually, it's useful to clients but not only. In my use case, MPD is > > on a headless server. Without it, it's not possible to receive this > > stream in Javascript context due to Cross Origin protection in > > browsers. That prevents using HTTPD plugin as a audio source for a web > > page, unless you have the Allow Origin header (ditto for > > Icecast/Shoutcast mode). > > An option for generating that one header sounds acceptable. But what > you wrote is hard to understand, hard to configure and error prone. > ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/03 23:09, X Ryl wrote: > Actually, it's useful to clients but not only. In my use case, MPD is > on a headless server. Without it, it's not possible to receive this > stream in Javascript context due to Cross Origin protection in > browsers. That prevents using HTTPD plugin as a audio source for a web > page, unless you have the Allow Origin header (ditto for > Icecast/Shoutcast mode). An option for generating that one header sounds acceptable. But what you wrote is hard to understand, hard to configure and error prone. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
> How is this change related to your patch description? > > -# encoder "vorbis"# optional, vorbis or lame > +# encoder "vorbis"# optional, vorbis or lame or > any encoder (use mpd --verbose to get a list) It's not. The comment is wrong, and is 2 lines above my change. I fixed it since I was here, can revert it, but I'm not going to make a patch for this one (false) line. > > Still no documentation, and still no reason why this feature is > useful. I don't like it at all, and the commit message needs to > convince me. Actually, it's useful to clients but not only. In my use case, MPD is on a headless server. Without it, it's not possible to receive this stream in Javascript context due to Cross Origin protection in browsers. That prevents using HTTPD plugin as a audio source for a web page, unless you have the Allow Origin header (ditto for Icecast/Shoutcast mode). The usual workaround, is to serve the music library out of MPD via the same port as the webserver, but it's bad because: 1) It duplicates the effort (what is the point of a music daemon if we have to use another software doing 80% of the same work ?) 2) No synchronization of "meta action". If I create a playlist in MPD it's not in the second software so I have to redo it. 3) Need to redo in Javascript what MPD does very well (like crossfading, replay gain, mixramp etc...) 4) Some feature will never be doable in Javascript/HTML, like input streaming from any sources. Also, the feature can be used for reverse proxy mode, identifying the source MPD when numerous are being used. Cheers, Cyril ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/03 22:00, X Ryl wrote: > Please find attached a new patch, with your other remarks fixed. How is this change related to your patch description? -# encoder "vorbis"# optional, vorbis or lame +# encoder "vorbis"# optional, vorbis or lame or any encoder (use mpd --verbose to get a list) This is the very first thing I see in your patch file. Still no documentation, and still no reason why this feature is useful. I don't like it at all, and the commit message needs to convince me. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
Ok. My intention was not to start a flamewar, but to contribute back a feature that's missing in the daemon. Actually you criticize that there was no documentation, but I do include the documentation, since I've added it in the first file (and by the way, I fixed the wrong comments in the file)! All the added members in classes have their doxygen documentation too. The "space" change you've spot is used to align vertically the documentation improvement I've added in the conf example file. I'll remove it, but it'll be likely ugly due to the new option name length that's 1 char larger than the previous ones... For verification, I see no code in the current code base for testing the other parameters from the HTTPOutput plugin, if there was some, I would have appended mine here. I'm probably wrong, so please spot where I should improve the testing code. I see test_ files in Test folder, but none of them test the tokenizer. Please find attached a new patch, with your other remarks fixed. Cheers, Cyril 2014-12-03 21:23 GMT+01:00 Max Kellermann : > On 2014/12/03 17:00, X Ryl wrote: >> I don't see a single whitespace change in this patch. > > I do. Proof: > > -# type"httpd" > +# type "httpd" > >> Also, I didn't know what were the project policies, but the content of >> the initial email is summed up in the commit message on top of the >> patch, >> Maybe you want me to send the patch as embedded text in this mail, or >> with attachment is enough ? > > No, I just want NO EXPLANATION in the email. Explanation outside of > the commit message as an indication for a bad commit message. And > indeed it is bad; it contains everything in one huge subject line. > > There is no documentation, and there is no verification. Your > indentation is wrong (spaces instead of tabs). 0001-plugins-httpd-Add-support-for-optional-headers-in-HT.patch Description: Binary data ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/03 17:00, X Ryl wrote: > I don't see a single whitespace change in this patch. I do. Proof: -# type"httpd" +# type "httpd" > Also, I didn't know what were the project policies, but the content of > the initial email is summed up in the commit message on top of the > patch, > Maybe you want me to send the patch as embedded text in this mail, or > with attachment is enough ? No, I just want NO EXPLANATION in the email. Explanation outside of the commit message as an indication for a bad commit message. And indeed it is bad; it contains everything in one huge subject line. There is no documentation, and there is no verification. Your indentation is wrong (spaces instead of tabs). ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
Hi, I'm sorry, but I don't see a single whitespace change in this patch. Also, I didn't know what were the project policies, but the content of the initial email is summed up in the commit message on top of the patch, Maybe you want me to send the patch as embedded text in this mail, or with attachment is enough ? Regards, Cyril 2014-12-02 21:40 GMT+01:00 Max Kellermann : > On 2014/12/02 14:05, X Ryl wrote: >> As the subject says, this patch adds support for optional headers for >> the HTTP output plugins. These headers are required for allowing cross >> origin resource sharing (for one, but it can be used in reverse proxy >> mode to identify the server sources). >> This makes possible to receiving the HTTP stream asynchronously in >> Javascript, and as such to lower the latency when playing the stream >> in a web browser / javascript based audio player. >> >> It also fix a bug with escaped chars not processed as described in the >> config file's tokenizer. > > I find it too difficult to read your patch, due to many pointless > whitespace changes. Please strip those out. > > Oh, and a patch submission should not need an explanation email. If > you need email text, then your commit message is too short. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
Re: [mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
On 2014/12/02 14:05, X Ryl wrote: > As the subject says, this patch adds support for optional headers for > the HTTP output plugins. These headers are required for allowing cross > origin resource sharing (for one, but it can be used in reverse proxy > mode to identify the server sources). > This makes possible to receiving the HTTP stream asynchronously in > Javascript, and as such to lower the latency when playing the stream > in a web browser / javascript based audio player. > > It also fix a bug with escaped chars not processed as described in the > config file's tokenizer. I find it too difficult to read your patch, due to many pointless whitespace changes. Please strip those out. Oh, and a patch submission should not need an explanation email. If you need email text, then your commit message is too short. ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
[mpd-devel] PATCH: Add support for configuration specified optional headers in HTTP output plugins
As the subject says, this patch adds support for optional headers for the HTTP output plugins. These headers are required for allowing cross origin resource sharing (for one, but it can be used in reverse proxy mode to identify the server sources). This makes possible to receiving the HTTP stream asynchronously in Javascript, and as such to lower the latency when playing the stream in a web browser / javascript based audio player. It also fix a bug with escaped chars not processed as described in the config file's tokenizer. Best regards, Cyril 0001-Add-support-for-optional-headers-in-HTTPD-output-mod.patch Description: Binary data ___ mpd-devel mailing list mpd-devel@musicpd.org http://mailman.blarg.de/listinfo/mpd-devel
