Skip to content

FIX: Taglib: Problem reading embedded covers #1554

Merged
merged 1 commit into from Oct 7, 2012

2 participants

@koying
Team Kodi member
koying commented Oct 6, 2012

I've noticed taglib couldn't read the music covers.

So, I've build taglib myself and tested with MinGW and have empirically found out that this patch solves the problem.

I thinks the fix is due to using the "const" version of ID3v2::AttachedPictureFrame::picture()->data() rather than the non-const one, which does a "detach()", but I'm not sure.

@night199uk

I don't understand how this fixes anything, the two calls should be the same.
can you do a little more digging here? i'm happy to merge it if you can show exactly why the two calls are different and how this fixes anything.

also, i don't have any issues reading cover art here on osx, and i've not heard any other reports of issues - but this code is still pretty recent so i'm willing to accept someone might not have stumbled on it.

@night199uk night199uk was assigned Oct 7, 2012
@koying
Team Kodi member
koying commented Oct 7, 2012

I don't understand either :)
I tried both VC10 + taglib from xbmc and Mingw + self-compiled taglib trunk and the results were similar in that the data pointer was invalid.

I'll try to dig out the root cause...

@koying
Team Kodi member
koying commented Oct 7, 2012

I think it is due to this, in taglib:

ByteVector AttachedPictureFrame::picture() const
{
  return d->data;
}

IMO, It should return a ByteVector&. The returned ByteVector is a shallow copy allocated on stack and gets deleted after the assignation to "data". OTOH, due to the "detach()" in "data()", the embedded ByteVectorPrivate is a deep copy with a refcount of 1, which gets deleted by the "deref()" in its parent dtor, leading to an invalid pointer.

I assume it works on unixes due to different allocation strategies, leading to the memory block being still untouched for the "art->set()". I tested this on MinGW, so it cannot be due to a different compiler.

@night199uk

agreed. I've reported this to the taglib maintainer to see what they say. I'm thinking to work around it in a slightly different way. Can you tell me if this works for you?

/*
* picture() returns a copy, should return a ref.
* when we call .data() on the stack copy of the bytevector
* it can be freed before we use the pointer.
* remove when taglib gets fixed to return a reference.
*
/
ByteVector picture = pictures[i]->picture();
string mime = pictures[i]->mimeType().toCString();
TagLib::uint size = picture.size();
uint8_t
data = (uint8_t*) picture.data();
tag.SetCoverArtInfo(size, mime);
if (art)
art->set(data, size, mime);

@koying
Team Kodi member
koying commented Oct 7, 2012

I cannot test right now but it seems ok (besides the uint8_t* data). Will test tomorrow to be sure.
I don't think it's worth the hassle, though, as "data" is only used once and my way won't necessitate to reverse code when upstream is corrected.

Another way would be to somehow force the use of the const version of "data()", which wouldn't detach and thus the block wouldn't be freed.

@night199uk

yes, you're right. merging this.

@night199uk night199uk merged commit 17cf5e4 into xbmc:master Oct 7, 2012
@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request May 25, 2015
@LongChair LongChair Remive IncludeRetaled Option for URI Items, fixes #1554 ac5b8a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.