[vlc-devel] [PATCH] Add module to submit listens to ListenBrainz.

Mon Apr 6 12:31:33 CEST 2020

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.

> - 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.

> - 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">
> <html><head>
> <title>404 Not Found</title>
> </head><body>
> <h1>Not Found</h1>
> <p>The requested URL was not found on this server.</p>
> <hr>
> <address>Apache/2.4.38 (Debian) Server at gllm.fr Port 443</address>
> </body></html>
>
>
> 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...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200406/6e2f23ba/attachment.html>