Implement Track Metadata via C++ class

mill1000 commented 4 years ago

Oops looks like we've done some duplicate work. I'm creating this as a place to discuss.

Support referencing by enum and name
Direct access to const std::string values
DIDL-Lite XML generated via pugixml library
Added support for track number and date tags

A big part of this PR would be the discussion around the XML library. Pugixml seemed like a good fit to me. Native C++ lib with decent documentation and no complicated build required.

Luckily it's implemented such that if we were to change XML libs only CreateRootXml and ToXml should be effected.

mill1000 commented 4 years ago

Looks like you've started to make some changes to TrackMetadata. I had also worked to implement it as a class.

hzeller commented 4 years ago

I think the upnp library also has some XML functionality built-in, but not sure if it is only parsing or also DOM-modifying. But if so, I'd suggest to explore that possibility for XML editing.

I am reluctant to add 14000 lines of pugixml dependency (in a project that is less than 7000 lines otherwise) just to save maybe 50 lines of code...

mill1000 commented 4 years ago

Luckily not all the lines compile in :) but it did have a larger impact on the final code size then I would have expected.

I whipped up a variation here (mill1000/gmrender-resurrect@48b0aa946966e0a6fc96800158cc2184d6249bd6) that uses leethomason/tinyxml2 instead. This one only adds about 5k lines.

mill1000 commented 4 years ago

Quick comparison of the 3 XML options mentioned so far.

iXML

Pros

Integrated into libupnp
Portions of the libupnp API return iXML structures so it's unavoidable at times.
Cons
C-style API -> manual memory management, no templated or overloaded methods
Documentation is buried within libupnp releases

pugixml

Pros

Nice C++ API with overloaded set operations.
Fails gracefully when nodes are non-existent. (e.g. xml_document.child("DIDL-Lite").child("item") collapses to nullptr without having to check intermediate results)
API chains well together (e.g. An attribute can be added and set at the same time item.append_attribute("id").set_value(""))
Automatic character escapes for LastChange variable
Well documented with usage examples
Cons
Heaviest (in code size) of the 3 options
Outputting to a string occurs via stringstream which is heavier.
A tad pedantic about node children/values (e.g. you have to append a node_pcdata before you can set the tag value).

TinyXML2

Pros

C++ API with overloaded set operations. It does however feel a little Java-ish
Smaller than pugixml. No use of stringstream either.
Simpler API for setting node attributes and values. (e.g. item->SetAttribute("id", "") and element->SetText("Some value"))
Automatic character escapes
Documentation is present but mostly just a class hierarchy.
Cons
API requires passing around the parent XML document
Doesn't chain as well. Elements are created and appended in separate calls. (e.g. root->InsertEndChild(document->NewElement("child"))->ToElement()->SetText("value");)
API relies on pointers and doesn't fail gracefully so nullptr checks are a must for each access.

hzeller commented 4 years ago

If we get pugixml or tinyxml2 just as debian development dependency, maybe potential costs in terms of 'heavy' dependency is somewhat small ?

In general I am a friend of adding dependencies only when needed, so I started playing around making a small c++ layer around the iXML thing we get from libupnp. Right now only parsing just to see 'how it looks'. It supports simple chaining, so access even with null-elements in the path is readable. (example: https://github.com/hzeller/gmrender-resurrect/blob/experimental-xmldoc2/src/track-meta-data.cc#L104

full branch: https://github.com/hzeller/gmrender-resurrect/tree/experimental-xmldoc2

new stuff is in xmldoc2.{h,cc} )

mill1000 commented 4 years ago

pugixml or tinyxml2 just as debian development dependency

Brilliant thought! Both pugixml and tinyxml2 are available as debian (& raspbian) packages. 👍

In general I am a friend of adding dependencies only when needed

And in general I'd say I don't like to reinvent the wheel. I'll check out your branch though!

whyman commented 4 years ago

I would suggest that using a third party library would be the way to go. Recently over at Gerbera we have switched to pugixml as part of our move towards more modern standardised data-structures and libraries.

For me the best point was that community members are more likely to know pugixml than libupnp's own "interesting" implementation.

In this day and age of 100Mb Go binaries, I dont think anyone will care about the slightly larger filesize.

hzeller commented 4 years ago

For now, I've added a little wrapper around the upnp xml lib that has just the bare minimum things we need ( https://github.com/hzeller/gmrender-resurrect/blob/c%2B%2B-conversion/src/xmldoc.h ) and does the trick for now (now merged to the c++-conversion branch). While the libupnp xml itself is indeed somewhat ugly, this little abstraction layer makes similar to use as other modern libs. If this brings us into trouble, we should consider 3rd-party libs, but luckily we only really have to deal with XML in the ugly parts of UPnP interaction.

Uh, and Gerbera looks great @whyman , I was not aware of it (I still have some somewhat old, but reliable minidlna set-up, but that project is in maintenance apparently and occasional requests are also not really merged).

mill1000 commented 4 years ago

Closing this PR. I re-worked what I had here to use the XMLDoc wrapper in PR #206.

The XMLDoc wrapper works well enough but there are some oddities that should be addressed.

hzeller / gmrender-resurrect