Add salt attribute to APIC frames to avoid mangling descriptions and to allow identical descriptions

[obskyr]

Currently, Mutagen ever so slightly mangles files that contain multiple APICs with the same description: It appends spaces to the descriptions to make them unique.

mutagen/mutagen/id3/_frames.py

Lines 1277 to 1279 in 62a7b3e

	def _merge_frame(self, other):
	other.desc += u" "
	return other

This means that at the time of writing, it’s impossible to tag files with images with the same description using Mutagen. To allow this, this pull request adds a salt attribute to the APIC frame

This should, as far as I know, be entirely backward-compatible, as the HashKey doesn't change.

---

Since I was updating the documentation of APIC, I also took the liberty of removing the line “Mutagen will automatically compress large images when saving tags.”, since this appears to no longer be the case:

mutagen/mutagen/id3/_tags.py

Lines 513 to 520 in 62a7b3e

	usize = len(framedata)
	if usize > 2048:
	# Disabled as this causes iTunes and other programs
	# to fail to find these frames, which usually includes
	# e.g. APIC.
	# framedata = BitPaddedInt.to_str(usize) + framedata.encode('zlib')
	# flags |= Frame.FLAG24_COMPRESS | Frame.FLAG24_DATALEN
	pass

---

The documentation for this addition looks as follows:

[phw]

This is interesting. I never realized this issue existed. But I checked the code in MusicBrainz Picard, and it applies a workaround of adding a counter to the description.

I wonder whether the hash could be set to a value automatically. Could we set it to something? Maybe something based on the image data, or even just a random unique string? As I understand it when adding a new APIC object to the tags the expectation would be that this gets stored separately, even if the other data is the same.

[obskyr]

We could technically base it on the image data, but to be certain of uniqueness we'd have to hash the whole image, which would add potentially significant amounts of runtime with no particularly significant benefit. (There'd be the option to only hash part of the image, but… then the question becomes, where to draw that line?)

Now, it could be set to a random UUID, but then we lose the property that images have the same, predictable hash no matter how many times you reload the file – which could be helpful (and potentially already be an assumption that user code in the wild acts under).

One potential approach I’m playing with in my head, however, is that the HashKey of an APIC could be set to include the index of the APIC – so the first APIC would have the HashKey APIC:1, the second would have APIC:2, and so on. Only issue with that is, there could be code out there that relies on the property that the image description is part of the HashKey – it’s arguably slightly API-breaking, even if it is a much nicer API. What do you think?

[phw]

Those are good points.I agree that it is slightly API breaking if the hash gets preset. Also tools having run into this issue likely already apply some workaround as Picard does.

Let's play it safe and let the caller set the hash if needed.

In regards to your other PR that will have a merge conflict: Shall we merge this and rebase the other?

[obskyr]

Sure, let’s do it that way! Though does that have a merge conflict with this…? I don’t recall that being the case, but I might be misremembering.

[phw]

No, of course it doesn't conflict. My bad. For me this looks good to merge.

@lazka As this is technically an API extension, could you have a final look before we merge?