[vlc-devel] [PATCH] Add module to submit listens to ListenBrainz.
thomas at gllm.fr
Mon Apr 6 13:04:13 CEST 2020
On Mon, Apr 6, 2020, at 12:31, Kartik Ohri wrote:
> On Mon, Apr 6, 2020 at 1:56 PM Thomas Guillem <thomas at gllm.fr> wrote:
>> I tested your plugin and found some issues:
>> *First, the issues I fixed (cf. the attached patch):*
>> - Changed variables names: we prefer to use "-" as a separator for VLC variables.
>> - Fixed a unused variables warning ((vlc_tick_t system_date , void *data).
>> - You should handle AlbumArtist first, then Artist if not present
>> - Fixed the creads leak.
>> - Used VLC returns code for error handling
>> - Fixed lot of memory leaks. You were leaking all the metadata of the songs.
>> - Fixed a thead issue: you were clearing the queue from SendRequest() while the lock was not hold. That is why I moved the mutexes locking/unlocking a little bit.
> Thanks for the changes.
>> *and the issues I didn't fix:*
>> - The first song is always missed because of missing Artist. I think it's because you forgot to listen to the on_media_meta_changed player event.
> This was not the case when I tested it. I have tested again after applying your patch. The first song is not missed because of missing Artist. The log message for Missing artist is made due to two calls to Enqueue. I will find the cause of this extra call and fix it.
It's a threading issue, when you receive events the media can be parsed or not, you can't know. It will depends on the media format type or number of cores of your device. That is why you should listen to the meta_changed event.
>> - Why are you encoding everything when storing on the listen_t struct, and then decoding everything before sending it ?
> This is because of some UTF errors I was facing on server side otherwise while sending the JSON payload. Encoding the metadata fixed the issue but replaced spaces as well. So I decoded the metadata before sending it.
It need to be documentend then. And why not doing this hack in one place ? Encode, then decode from the same function.
But then again, this hack looks overkill. I'm sure VLC has other string helper that can encode or escape required characters.
>> - The http response parsing is not robust enought. I was able to get all request to succeed by just changing the API url to my server that doesn't implement the listbrainz protocol. Here is the response I got.
>>> HTTP/1.1 404 Not Found
>>> Date: Mon, 06 Apr 2020 07:32:38 GMT
>>> Server: Apache/2.4.38 (Debian)
>>> Strict-Transport-Security: max-age=15552000
>>> Content-Length: 270
>>> Connection: close
>>> Content-Type: text/html; charset=iso-8859-1
>>> <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
>>> <title>404 Not Found</title>
>>> <h1>Not Found</h1>
>>> <p>The requested URL was not found on this server.</p>
>>> <address>Apache/2.4.38 (Debian) Server at gllm.fr Port 443</address>
>> As you can see, there is a "200" in this responses, hence the success.
>> You should only parse the line "HTTP/1.1 404 Not Found".
> Thanks for pointing this out. I'll fix this.
>> Best regards,
>> Thomas Guillem
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the vlc-devel