Rewrite taglib writer to use property interface
ClosedPublic

Authored by astippich on Jan 29 2019, 6:16 PM.

Details

Summary

Rewrite the taglib writer to use taglib's
unified property interface, which is more extensible.

Test Plan

tests pass

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Jan 29 2019, 6:16 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptJan 29 2019, 6:16 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Jan 29 2019, 6:16 PM
bruns added inline comments.Jan 31 2019, 6:09 PM
src/writers/taglibwriter.cpp
67–68

use a smart pointer (unique_ptr, QScopedPtr) here, and get rid of the delete below.

bruns added a comment.Jan 31 2019, 6:48 PM

I think this becomes better structured when you:

  1. Create one function per file type, with parameters (Taglib::Filestream, KFM::PropertyMap)
  2. From this function, call a generic updateProperties(oldProperties, newProperties) -> mergedProperties
  3. Call the type specific function from TaglibWriter::write(...)

Especially when taking the changes for writing the rating into account, this would make the code easier to read - handling of different types just once (not once for reading and once for writing), and no upcasting/dynamic_cast of Taglib::File*. It also saves the heap allocation of the concrete TagLib::File implementation.

astippich updated this revision to Diff 50704.Feb 2 2019, 10:44 AM
  • rewrite for better readability and to avoid heap allocation

I think this becomes better structured when you:

  1. Create one function per file type, with parameters (Taglib::Filestream, KFM::PropertyMap)
  2. From this function, call a generic updateProperties(oldProperties, newProperties) -> mergedProperties
  3. Call the type specific function from TaglibWriter::write(...)

    Especially when taking the changes for writing the rating into account, this would make the code easier to read - handling of different types just once (not once for reading and once for writing), and no upcasting/dynamic_cast of Taglib::File*. It also saves the heap allocation of the concrete TagLib::File implementation.

Done. It requires a little bit more of boilerplate code, but I have not strong preference.

bruns added inline comments.Feb 4 2019, 9:31 PM
src/writers/taglibwriter.cpp
70–73

When you make this
void writeGenericProperties(Taglib::File *file, const PropertyMap &newProperties), you can do file->properties(); {/* merge */}; file->setProperties(...); here, saving most of the duplicate code below.

dito for the specializations in D18604, just pass in Taglib::File*, and call auto tags = dynamic_cast<FooTag*>(file->tag()); there.

astippich added inline comments.Feb 5 2019, 6:49 PM
src/writers/taglibwriter.cpp
70–73

That will then require to load and write the property map twice when properties only specific to some tagging formats need to be written, see e.g. Ape and Vorbis tags in D18604. I would like to avoid this.

bruns added inline comments.Feb 28 2019, 10:53 PM
src/writers/taglibwriter.cpp
70–73

So iff the rating is updated at the same time as another property, taglib has to adjust some in-memory structure twice. Obviously, for most formats this poses no problem, but for Ape/Vorbis it does?

Also, this no longer applies when you use the specific tag types for Ape/Ogg.

Writing to disk only happens when calling file->save()...

astippich added inline comments.Mar 1 2019, 11:20 AM
src/writers/taglibwriter.cpp
70–73

Not sure I understand. I would like to avoid doing file->properties(); and file->setProperties(); twice for those formats for which only the PropertyMap is sufficient, e.g. Ape and Ogg, to avoid doing any unnecessary work. The beauty of Ape and Ogg in the TagLib implementation is that they solely work with the PropertyMap and do not require any special handling. See D18826, Ape and Ogg do not have any extra codepath at all. For writing tags, we have to be a little bit more careful, though. If writing Rating information is added to writeGenericProperties, for example Id3v2's "TXXX" tags will be polluted with these values.

bruns added inline comments.Mar 1 2019, 7:29 PM
src/writers/taglibwriter.cpp
70–73

I think for reading it as generic as possible is fine, but for writing being a little bit more explicit does not hurt.

My proposal for Ape and Ogg is to split the writing for these to two different functions. Yes, it is possible to use the propertymap for both, as both use the same scale and the same tag name, but the implementations on the taglib side are completely different.

Writing the "rating" for XiphComment (Ogg) and Ape in distinct functions (see my comment in D18604) hardly adds any code, but gets the Ape and Ogg code paths in line with the other file formats. You don't write the ASF/MP4 rating using the property interface although it would be possible, and IMHO thats the right thing to do, also for Ape and Ogg.

As soon Ape and Ogg are split, you no longer rely on the PropertyMap for the rating, and you won't have to use properties()/setProperties() twice.

astippich added inline comments.Mar 8 2019, 1:35 PM
src/writers/taglibwriter.cpp
70–73

No, I cannot use the the PropertyMap for ASF/MP4, those atoms/attributes are unsupported in the PropertyMap and need to be handled separately. I would have done so if it is possible.
I really do not like to write unnecessary, duplicated code when TagLib handles this nicely for us. I've only chosen the rating property to be implemented first, but there are more to come. The code will be 100% the same for APE and OGG with the PropertyMap, and also more efficient as we query the PropertyMap anyway.

bruns added inline comments.Mar 8 2019, 4:25 PM
src/writers/taglibwriter.cpp
70–73

Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* unknown key and uses it verbatim, while for MP4 and ASF it rejects any unknown key.

The setProperties() is also quite inconsistent, for APE and ASF it only removes items which have an empty value, while for Xiph, the properties are completely replaced.

As soon as you add support for a property where APE and Xiph key naming differs, or is only supported by one, you will require two functions anyway.

Using APE::Tag::setItem(...) is as efficient as manipulating the key/value in the Taglib::PropertyMap first and setting it by setProperties(...). Likewise for Xiph.

If you want to squeeze out the last bit of efficiency, you would skip the setProperties(...) completely when no property is changed by writeGenericProperties(....). This happens e.g. if you only change the rating.

If you want to avoid duplicate code, move the properties()/setProperties() into writeGenericProperties(), that saves 12*2 lines and adds 2.

astippich added inline comments.Mar 10 2019, 3:27 PM
src/writers/taglibwriter.cpp
70–73

Taglib does not "handle this nicely". For APE and Xiph, it just accepts *any* unknown key and uses it verbatim, while for MP4 and ASF it rejects any unknown key.

And so do APE::Tag::setItem and OGG::XiphComment::addField ...? btw, also perfectly legal, APE and Xiph allow writing arbitrary tags, while the others do not.

The setProperties() is also quite inconsistent, for APE and ASF it only removes items which have an empty value, while for Xiph, the properties are completely replaced.

Xiph explicitly allows multiple entries per key, which need to be removed when writing.

As soon as you add support for a property where APE and Xiph key naming differs, or is only supported by one, you will require two functions anyway.

TagLib automatically translates different keys from APE to "common names", e.g. DISC->DISCNUMBER etc.

I would really like to hand off manual tag handling to TagLib as much as possible. The library solely responsible for reading tags usually knows better how to handle the tags than we do (with a few exceptions to the rule of course).

bruns added inline comments.Mar 10 2019, 10:49 PM
src/writers/taglibwriter.cpp
70–73

By using the type specific function you signal you are aware of the differences between the two, and supply the appropriate data.

RATING is not handled by the properties interface, it just works by coincidence, not by design.

In case Taglib properly handles a tag, I am not against using it, as said several times. This is the case for DISC, but not for RATING.

bruns requested changes to this revision.Mar 22 2019, 10:13 PM
This revision now requires changes to proceed.Mar 22 2019, 10:13 PM
astippich updated this revision to Diff 55125.Mar 31 2019, 11:56 AM
  • check that file is valid
  • rebase
bruns accepted this revision.Mar 31 2019, 2:44 PM
This revision is now accepted and ready to land.Mar 31 2019, 2:44 PM
This revision was automatically updated to reflect the committed changes.