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

Mon Apr 6 13:22:01 CEST 2020

On Mon, Apr 6, 2020 at 4:34 PM Thomas Guillem <thomas at gllm.fr> wrote:

>
>
> 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.
>
> I found the cause of the extra call. When on_current_media_changed
callback is called, it calls Enqueue on the previous media which is NULL
for the first media. The first media is actually submitted when a second
media is added or the first one is stopped. This can be fixed with a simple
NULL check. But as per your suggestions, it definitely makes sense to use
the meta_changed event. I will look into it and use 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.
>
>
> 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.
>
I'll fix these issues. I'll try to find a less expensive hack that works
out.

>
>
>
>
> - 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/9ed875b7/attachment.html>