Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup taglibtagloader a bit and fix null padded tags #9788

Merged
merged 2 commits into from May 16, 2016
Merged

Conversation

@Paxxi
Copy link
Member

Paxxi commented May 9, 2016

Hide taglib includes from rest of Kodi code and move some helper methods into cpp file

@Paxxi

This comment has been minimized.

Copy link
Member Author

Paxxi commented May 9, 2016

@Razzeee @DaveTBlake review, maybe not the cleanest changeset but changes should be good

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 11, 2016

Thanks @Paxxi, I've been off-line a couple of days and won't be able to check this fully until Friday. It is odd how the null padding issue related to our string handler didn't show up in 15 or 16. Do you understand why that was?

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented May 11, 2016

Can you split the fix from the cleanup - commit wise

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 13, 2016

As I understand it the fix to nul padded ID3v1 tags problem is the removeal of TagStringHandler, a separate commit would make that clearer. :)

But what I don't understand is why exactly the same TagStringHandler code works in Jarvis. What else changed with 17 that made this an issue? The Taglib guys think our code would have had the same issue with 1.9.1, so the change to 1.10 is not the cause. Was it the VS2015 change? I guess I would need to compile Taglib 1.9.1 in VS2015 to know. @Paxxi do you understand it?

@Paxxi

This comment has been minimized.

Copy link
Member Author

Paxxi commented May 13, 2016

I'll make a separate commit. No I haven't dug that deep into it but there's not any reason to use our string handling as taglib does conversions itself so I just dropped it

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 13, 2016

No need to dig @Paxxi I found my explanation and can be at peace. There was a change in taglib 1.10 to the way tstring.cpp String::copyFromUFT8 worked, hence how the nul padding issue appeared.

Good to drop the unnecessary and broken string handling, and thanks for the tidy-up. I will give it a bit more testing tomorrow (managed to break my dev setup).

Hide taglib includes from rest of Kodi code and move some helper methods into cpp file
@Paxxi Paxxi force-pushed the Paxxi:tagloader branch from 8565939 to 0b64ac4 May 13, 2016
@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 14, 2016

Thanks for splitting the commits.
The latter fixes the singles (music files with ID3v1 tags and not album title) appearing as albums issue reported in http://trac.kodi.tv/ticket/16454 caused by those nul padded strings.

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 14, 2016

jenkins build this please

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 14, 2016

@Paxxi nice crop of errors on all platforms except Win32, care to fix them please.
Not sure why you changed those private methods to functions, and the other compilers don't seem to like it.

@Paxxi Paxxi force-pushed the Paxxi:tagloader branch from 0b64ac4 to cdabf9d May 14, 2016
@Paxxi

This comment has been minimized.

Copy link
Member Author

Paxxi commented May 14, 2016

jenkins build this please

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 15, 2016

@Paxxi jenkins finally attempted this build, but still fails on multiple platforms.

@Paxxi

This comment has been minimized.

Copy link
Member Author

Paxxi commented May 15, 2016

jenkins build this please
I don't think it picked up the changes judging by the errors

@Paxxi

This comment has been minimized.

Copy link
Member Author

Paxxi commented May 15, 2016

@DaveTBlake there we go, all good

@DaveTBlake

This comment has been minimized.

Copy link
Member

DaveTBlake commented May 16, 2016

Thanks @Paxxi. Tests fine for me too, do you want to hit the button or shall I?

@Paxxi Paxxi merged commit c5c4649 into xbmc:master May 16, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
default Merged build finished.
Details
@Paxxi Paxxi deleted the Paxxi:tagloader branch May 16, 2016
@hudokkow hudokkow added this to the Krypton 17.0-alpha2 milestone May 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.