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

Mon Apr 6 10:26:18 CEST 2020

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.

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

- Why are you encoding everything when storing on the listen_t struct, and then decoding everything 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".

Best regards,
Thomas Guillem
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200406/c03e2cae/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-listenbrainz-various-fixes.patch
Type: text/x-patch
Size: 10407 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200406/c03e2cae/attachment.bin>