<div dir="ltr"><div dir="ltr">On Mon, Apr 6, 2020 at 4:34 PM Thomas Guillem <<a href="mailto:thomas@gllm.fr">thomas@gllm.fr</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u><div><div><br></div><div><br></div><div>On Mon, Apr 6, 2020, at 12:31, Kartik Ohri wrote:<br></div><blockquote type="cite" id="gmail-m_8432184282933592068qt"><div dir="ltr"><div dir="ltr">On Mon, Apr 6, 2020 at 1:56 PM Thomas Guillem <<a href="mailto:thomas@gllm.fr" target="_blank">thomas@gllm.fr</a>> wrote:<br></div><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><u></u><br></div><div><div>I tested your plugin and found some  issues:<br></div><div><div><div><br></div></div><div><b>First, the issues I fixed (cf. the attached patch):</b><br></div><div><div><br></div></div><div>- Changed variables names: we prefer to use "-" as a separator for VLC variables.<br></div><div>- Fixed a unused variables warning ((vlc_tick_t system_date , void *data).<br></div><div>- You should handle AlbumArtist first, then Artist if not present<br></div><div>- Fixed the creads leak.<br></div><div>- Used VLC returns code for error handling<br></div><div>- Fixed lot of memory leaks. You were leaking all the metadata of the songs.<br></div><div>- 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.<br></div><div><div><br></div></div></div></div></blockquote><div>Thanks for the changes. <br></div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><div><div><br></div></div><div><b>and the issues I didn't fix:</b><br></div></div><div><br></div><div>- 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.<br></div><div><br></div></div></blockquote><div>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.<br></div></div></div></blockquote><div>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. </div><div><br></div></div></blockquote><div>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.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div></div><div><br></div><div><br></div><blockquote type="cite" id="gmail-m_8432184282933592068qt"><div dir="ltr"><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br></div><div>- Why are you encoding everything when storing on the listen_t struct, and then decoding everything before sending it ?<br></div><div><br></div></div></blockquote><div>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.<br></div></div></div></blockquote><div><br></div><div>It need to be documentend then. And why not doing this hack in one place ? Encode, then decode from the same function.<br></div><div>But then again, this hack looks overkill. I'm sure VLC has other string helper that can encode or escape required characters. </div></div></blockquote><div>I'll fix these issues. I'll try to find a less expensive hack that works out. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br></div><div><br></div><div><br></div><blockquote type="cite" id="gmail-m_8432184282933592068qt"><div dir="ltr"><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br></div><div>- 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.<br></div><blockquote type="cite"><div><br></div><div>HTTP/1.1 404 Not Found<br></div><div>Date: Mon, 06 Apr 2020 07:32:38 GMT<br></div><div>Server: Apache/2.4.38 (Debian)<br></div><div>Strict-Transport-Security: max-age=15552000<br></div><div>Content-Length: 270<br></div><div>Connection: close<br></div><div>Content-Type: text/html; charset=iso-8859-1<br></div><div><br></div><div><!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN"><br></div><div><html><head><br></div><div><title>404 Not Found</title><br></div><div></head><body><br></div><div><h1>Not Found</h1><br></div><div><p>The requested URL was not found on this server.</p><br></div><div><hr><br></div><div><address>Apache/2.4.38 (Debian) Server at <a href="http://gllm.fr" target="_blank">gllm.fr</a> Port 443</address><br></div><div></body></html><br></div></blockquote><div><br></div><div>As you can see, there is a "200" in this responses, hence the success.<br></div><div><br></div><div>You should only parse the line "HTTP/1.1 404 Not Found".<br></div><div><br></div></div></blockquote><div>Thanks for pointing this out. I'll fix this. <br></div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br></div><div>Best regards,<br></div><div>Thomas Guillem<br></div></div></blockquote></div></div></blockquote><div><br></div></div></blockquote></div></div>