Re: [emms-help] [PATCH] New metadata extraction program

[Rasmus]

Hi Petteri,

Thanks for your patches!

Petteri Hintsanen <address@hidden> writes:

> New program emms-print-taglib-metadata uses TagLib's C++ interface to
> extract track information.

This will still not extract e.g. albumartist from wma.  But this is a
limitation in taglib.

> ---
>  AUTHORS                            |  1 +
>  Makefile                           |  5 ++
>  lisp/emms-info.el                  | 14 +++++-
>  src/emms-print-taglib-metadata.cpp | 96 
> ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 114 insertions(+), 2 deletions(-)
>  create mode 100644 src/emms-print-taglib-metadata.cpp
>
> diff --git a/AUTHORS b/AUTHORS
> index 3111770..e84c4ab 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -27,6 +27,7 @@ Ye Wenbin            <address@hidden>
>  Yoni (Johnathan) Rabkin  <address@hidden>
>  mathias.dahl                  <mathias.dahl>
>  Rasmus Pank Roulund      <address@hidden>
> +Petteri Hintsanen        <address@hidden>

Maybe make this a separate commit.

>  ;; Local variables:
>  ;; coding: utf-8
> diff --git a/Makefile b/Makefile
> index c0846d6..dda3640 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -4,6 +4,8 @@ DOCDIR=doc/
>  LISPDIR=lisp
>  SRCDIR=src
>  
> +CXXFLAGS=-std=c++11

Would it be better to use old-style loops than making this requirement?
Personally, I think C++2011 is fine by now.

> +
>  ALLSOURCE=$(wildcard $(LISPDIR)/*.el)
>  ALLCOMPILED=$(wildcard $(LISPDIR)/*.elc)
>  
> @@ -37,6 +39,9 @@ docs:
>  emms-print-metadata: $(SRCDIR)/emms-print-metadata.c
>       $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -o $(SRCDIR)/$@ $< 
> `taglib-config --cflags --libs` -ltag_c
>  
> +emms-print-taglib-metadata: $(SRCDIR)/emms-print-taglib-metadata.cpp
> +     $(CXX) $(CPPFLAGS) $(CXXFLAGS) $(LDFLAGS) -o $(SRCDIR)/$@ $< 
> `taglib-config --cflags --libs`
> +


The naming is confusing.  emms-print-metadata is also using taglib, though
not the property list of the C++ binding.  I say we stick to one program.
The implementation language doesn’t matter to the end user and the
functionality is the same.

At the very least you need to point the config towards the new program.

>  install:
>       test -d $(SITELISP) || mkdir -p $(SITELISP)
>       test -d $(INFODIR) || install -d $(INFODIR)
> diff --git a/lisp/emms-info.el b/lisp/emms-info.el
> index cfc206b..46b8cd9 100644
> --- a/lisp/emms-info.el
> +++ b/lisp/emms-info.el
> @@ -1,6 +1,6 @@
>  ;;; emms-info.el --- Retrieving track information
>  
> -;; Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Free Software Foundation 
> Inc.
> +;; Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2015  Free Software 
> Foundation Inc.
>  
>  ;; Author: Jorgen Schaefer <address@hidden>
>  
> @@ -29,16 +29,26 @@
>  ;; The code will add info symbols to the track. The following symbols
>  ;; are defined:
>  
> -;; info-artist - string naming the artist
> +;; info-albumartist - string naming the album artist
> +;; info-albumartistsort - string key for album artist collation
> +;; info-artist - string naming the track artist
> +;; info-artistsort - string key for artist collation
>  ;; info-composer - string naming the composer
> +;; info-composersort - string key for composer collation
>  ;; info-performer - string naming the performer
>  ;; info-title - string naming the title of the song
> +;; info-titlesort - string key for title collation
>  ;; info-album - string naming the album
> +;; info-albumsort - string key for album collation
>  ;; info-tracknumber - string(?) naming the track number
>  ;; info-discnumber - string naming the disc number
>  ;; info-year - string naming the year
> +;; info-originalyear - string naming the original release year
> +;; info-date - string naming the release date
> +;; info-originaldate - string naming the original release date
>  ;; info-note - string of free-form entry
>  ;; info-genre - string naming the genre
> +;; info-label - string for the record label
>  ;; info-playing-time - number giving the seconds of playtime
>  
>  ;;; Code:
> diff --git a/src/emms-print-taglib-metadata.cpp 
> b/src/emms-print-taglib-metadata.cpp
> new file mode 100644
> index 0000000..9e3f1d7
> --- /dev/null
> +++ b/src/emms-print-taglib-metadata.cpp
> @@ -0,0 +1,96 @@
> +/* emms-print-taglib-metadata.cpp --- Info function for TagLib
> +   Copyright (C) 2015  Free Software Foundation, Inc.
> +
> +Author: Petteri Hintsanen <address@hidden>
> +
> +This file is part of EMMS.
> +
> +EMMS is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 3, or (at your option)
> +any later version.
> +
> +EMMS is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with EMMS; see the file COPYING.  If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +Boston, MA 02110-1301, USA.  */
> +
> +#include <taglib/fileref.h>
> +#include <taglib/tag.h>
> +#include <taglib/tpropertymap.h>
> +#include <iostream>
> +
> +static const std::vector<std::string> tags_to_extract = {
> +  "album",
> +  "albumsort",
> +  "albumartist",
> +  "albumartistsort",
> +  "artist",
> +  "artistsort",
> +  "composer",
> +  "composersort",
> +  "performer",
> +  "year",
> +  "originalyear",
> +  "date",
> +  "originaldate",
> +  "genre",
> +  "label",
> +  "title",
> +  "titlesort",
> +  "tracknumber",
> +  "discnumber"
> +};
> +
> +void print_tag (const TagLib::PropertyMap& tags, const std::string& tag);
> +
> +int
> +main (int argc, char* argv[])

Is it on purpose that you write int main in two lines?

> +{
> +  If (argc != 2)
> +    {
> +      std::cerr << "Usage: emms-print-metadata file.{mp3,ogg,flac}\n"
> +        "Other formats may work as well.\n";
> +      return 1;
> +    }
> +
> +  TagLib::FileRef file (argv[1]);
> +  if (file.isNull ()) return 1;

Should an helpful error/message be displayed here?

> +  const auto tags = file.file ()->properties ();
> +
> +  for (const auto& tag : tags_to_extract)
> +    {
> +      print_tag (tags, tag);
> +    }
> +
> +  int length = 0;
> +  if (file.audioProperties ())
> +    {
> +      auto properties = file.audioProperties ();
> +      length = properties->length ();
> +    }
> +  std::cout << "info-playing-time=" << length << std::endl;
> +
> +  return 0;
> +}
> +
> +void
> +print_tag (const TagLib::PropertyMap& tags, const std::string& tag)

Again, you have the return type and the fun name it on two lines.  I find
it weird, but it’s also a while since I read C so perhaps I’m just not
accustomed to this.

> +{
> +  TagLib::StringList values = tags[tag];
> +  if (!values.isEmpty ())
> +    {
> +      // TODO: Extract all values, not only the first one.  EMMS needs
> +      // to be extended to handle such values.
> +      auto value = values.front ();
> +      std::cout << "info-" << tag << "=" << value.to8Bit (true) << std::endl;
> +    }
> +}
> +
> +/* emms-print-taglib-metadata.cpp ends here. */

Looks good.  I will install and use the patches locally soon.

Thanks,
Rasmus

-- 
Not everything that goes around comes back around, you know