added: POC mp4 movie/episode tag support

[un1versal]

Backport from @notspiff

This adds basic mp4 embedded meta reading see http://forum.xbmc.org/showthread.php?tid=192982

any code changes will require a knowledgeable person guidance, Im just "the enterprising individual" putting this forward.

I can handle the git portions just fine.

[ace20022]

Are you willing/able to do some code changes?

[fritsch]

@ace20022 just review as you would normally do. I will see to give a helping hand in fixing the points you don't like.

[un1versal]

@ace20022 please review and @fritsch will kindly help me fix this to satisfaction. Someone just make this as a back-port from notspiff and make some comments based on the implementation proposed.

[topfs2]

Looks sane to me

[topfs2]

@ace20022 Afaict it seems worthy of being in Helix. I'll put approved label, if you feel that its something you'd like to deal with then your free to remove the label. Otherwise I'll merge it in a couple of days.

[ace20022]

This is really just a poc and bad design imo. A video files shouldn't be processed in a class for music files. I think adding a new directory xbmc/video/tags would be good. We could also add a mkv metadata parser in the future. I have not checked the logic itself at that moment...

[topfs2]

Don't' really follow? Its parsed in VideoInfoScanner by a taglib which cares nothing about what media its parsing tags from.

[topfs2]

We could obviously move Taglib out of music since its not bound to music, it was put there since we don't' 'have any other video tagloaders.

[ace20022]

Don't' really follow? Its parsed in VideoInfoScanner by a taglib which cares nothing about what media its parsing tags from.

795 out of 862 lines of that file ( xbmc/music/tags/TagLoaderTagLib.cpp) are related to music specific tag reading. For example it uses the namespace MUSIC_INFO etc. I just don't think that it's sane to add video tag processing to it. @fritsch thoughts?

[topfs2]

Not arguing that. Just think thats out of scope of this PR
Its progress, you add stuff until it feels wrong, then you refactor. If its a complete hack which will cause more problems than gains, then you refactor first. What this PR do is "add until it feels wrong" and it does so without really hacking overly much. Its mostly semantics. What we can do next is refactor. While we contemplate how to refactor it gives users great features.

Its a very naive abstraction in core, were we view music completely separate from video. This PR proofs that this is not true. Both mp4 and ogg containers cares little if its audio only or audio-video, it still contains tags. Without this PR we have no need to refactor, or change that view :)

We probably should refactor to something along the lines of a MediaTagLoader. Which probably even, in extension, could contain pictures and game tag sniffing.

With that said, I'm fine with this not being in Helix. And if your volunteering to do that refactor before I* that would be lovely, but would be nice if the refactoring doesn't delay this indefinitely.

[fritsch]

What @topfs2 says. Basically that PR does not hurt us. We get an additional dependency in the VideoInfoScanner that has "music" in the name, which yeah - feels like a more general refactor to be needed, to reduce included dependencies. I second @ace20022 oppinion in general.

Besides that it is nicely done and can be easily refactored later as it does not spam the TagLoader implementation. It has my +1 for inclusion cause of the mentioned reasons.

[ace20022]

Okay, I don't mind. Like I said before I can't comment on the changes to VideoInfoScanner.cpp, the if logic looks strange though. But I assume the two of you have checked that.

And if your volunteering to do that refactor before I* that would be lovely

No, thanks.

[MilhouseVH]

@UniversaI - can you look this thread over and see if it's relevant? It's possible this PR is causing a hang during scanning.