musikr: improve native error handling

Not an ideal error reporting system, but for the purposes of getting
4.0.0 out as fast as possible it will do.

musikr/src/main/cpp/JInputStream.cpp

@@ -22,14 +22,17 @@
 
 #include "JClassRef.h"
 #include "JByteArrayRef.h"
+#include "JStringRef.h"
 
 JInputStream::JInputStream(JNIEnv *env, jobject jInputStream) : env(env), jInputStream(
         jInputStream) {
     JClassRef jInputStreamClass = { env,
             "org/oxycblt/musikr/metadata/NativeInputStream" };
     if (!env->IsInstanceOf(jInputStream, *jInputStreamClass)) {
-        throw std::runtime_error("oStream is not an instance of TagLibOStream");
+        throw std::runtime_error("Object is not NativeInputStream");
     }
+    jInputStreamNameMethod = jInputStreamClass.method("name",
+            "()Ljava/lang/String;");
     jInputStreamReadBlockMethod = jInputStreamClass.method("readBlock",
             "(J)[B");
     jInputStreamIsOpenMethod = jInputStreamClass.method("isOpen", "()Z");
@@ -50,7 +53,9 @@ JInputStream::~JInputStream() {
 
 TagLib::FileName JInputStream::name() const {
     // Not actually used except in FileRef, can safely ignore.
-    return "";
+    JStringRef jName { env, reinterpret_cast<jstring>(env->CallObjectMethod(
+            jInputStream, jInputStreamNameMethod)) };
+    return jName.copy().toCString();
 }
 
 TagLib::ByteVector JInputStream::readBlock(size_t length) {

musikr/src/main/cpp/JInputStream.h

@@ -115,6 +115,7 @@ class JInputStream: public TagLib::IOStream {
 private:
     JNIEnv *env;
     jobject jInputStream;
+    jmethodID jInputStreamNameMethod;
     jmethodID jInputStreamReadBlockMethod;
     jmethodID jInputStreamIsOpenMethod;
     jmethodID jInputStreamSeekFromBeginningMethod;

musikr/src/main/cpp/JStringRef.cpp

@@ -19,6 +19,9 @@
 #include "JStringRef.h"
 #include "util.h"
 
+JStringRef::JStringRef(JNIEnv *env, jstring jString) : env(env), string(jString) {
+}
+
 JStringRef::JStringRef(JNIEnv *env, const TagLib::String string) {
     this->env = env;
     this->string = env->NewStringUTF(string.toCString(true));
@@ -28,6 +31,13 @@ JStringRef::~JStringRef() {
     env->DeleteLocalRef(string);
 }
 
+TagLib::String JStringRef::copy() {
+    auto chars = env->GetStringUTFChars(string, nullptr);
+    TagLib::String result = chars;
+    env->ReleaseStringUTFChars(string, chars);
+    return result;
+}
+
 jstring& JStringRef::operator*() {
     return string;
 }

musikr/src/main/cpp/JStringRef.h

@@ -24,6 +24,8 @@
 
 class JStringRef {
 public:
+    JStringRef(JNIEnv *env, jstring jString);
+
     JStringRef(JNIEnv *env, TagLib::String string);
 
     ~JStringRef();
@@ -32,6 +34,8 @@ class JStringRef {
 
     JStringRef& operator=(const JStringRef&) = delete;
 
+    TagLib::String copy();
+
     jstring& operator*();
 
 private:

musikr/src/main/cpp/taglib_jni.cpp

@@ -30,82 +30,174 @@
 #include "taglib/vorbisfile.h"
 #include "taglib/wavfile.h"
 
+std::unique_ptr<TagLib::FileRef> openFile(JNIEnv *env, jobject inputStream) {
+}
+
+bool parseMpeg(const char *name, TagLib::File *file,
+        JMetadataBuilder &jBuilder) {
+    auto *mpegFile = dynamic_cast<TagLib::MPEG::File*>(file);
+    if (mpegFile == nullptr) {
+        return false;
+    }
+    auto id3v1Tag = mpegFile->ID3v1Tag();
+    if (id3v1Tag != nullptr) {
+        try {
+            jBuilder.setId3v1(*id3v1Tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse ID3v1 tag in %s: %s", name, e.what());
+        }
+    }
+    auto id3v2Tag = mpegFile->ID3v2Tag();
+    if (id3v2Tag != nullptr) {
+        try {
+            jBuilder.setId3v2(*id3v2Tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse ID3v2 tag in %s: %s", name, e.what());
+        }
+    }
+    return true;
+}
+
+bool parseMp4(const char *name, TagLib::File *file,
+        JMetadataBuilder &jBuilder) {
+    auto *mp4File = dynamic_cast<TagLib::MP4::File*>(file);
+    if (mp4File == nullptr) {
+        return false;
+    }
+    auto tag = mp4File->tag();
+    if (tag != nullptr) {
+        try {
+            jBuilder.setMp4(*tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse MP4 tag in %s: %s", name, e.what());
+        }
+    }
+    return true;
+}
+
+bool parseFlac(const char *name, TagLib::File *file,
+        JMetadataBuilder &jBuilder) {
+    auto *flacFile = dynamic_cast<TagLib::FLAC::File*>(file);
+    if (flacFile == nullptr) {
+        return false;
+    }
+    auto id3v1Tag = flacFile->ID3v1Tag();
+    if (id3v1Tag != nullptr) {
+        try {
+            jBuilder.setId3v1(*id3v1Tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse ID3v1 tag in %s: %s", name, e.what());
+        }
+    }
+    auto id3v2Tag = flacFile->ID3v2Tag();
+    if (id3v2Tag != nullptr) {
+        try {
+            jBuilder.setId3v2(*id3v2Tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse ID3v2 tag in %s: %s", name, e.what());
+        }
+    }
+    auto xiphComment = flacFile->xiphComment();
+    if (xiphComment != nullptr) {
+        try {
+            jBuilder.setXiph(*xiphComment);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse Xiph comment in %s: %s", name, e.what());
+        }
+    }
+    auto pics = flacFile->pictureList();
+    jBuilder.setFlacPictures(pics);
+    return true;
+}
+
+bool parseOpus(const char *name, TagLib::File *file,
+        JMetadataBuilder &jBuilder) {
+    auto *opusFile = dynamic_cast<TagLib::Ogg::Opus::File*>(file);
+    if (opusFile == nullptr) {
+        return false;
+    }
+    auto tag = opusFile->tag();
+    if (tag != nullptr) {
+        try {
+            jBuilder.setXiph(*tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse Xiph comment in %s: %s", name, e.what());
+        }
+    }
+    return true;
+}
+
+bool parseVorbis(const char *name, TagLib::File *file,
+        JMetadataBuilder &jBuilder) {
+    auto *vorbisFile = dynamic_cast<TagLib::Ogg::Vorbis::File*>(file);
+    if (vorbisFile == nullptr) {
+        return false;
+    }
+    auto tag = vorbisFile->tag();
+    if (tag != nullptr) {
+        try {
+            jBuilder.setXiph(*tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse Xiph comment %s: %s", name, e.what());
+        }
+    }
+    return true;
+}
+
+bool parseWav(const char *name, TagLib::File *file,
+        JMetadataBuilder &jBuilder) {
+    auto *wavFile = dynamic_cast<TagLib::RIFF::WAV::File*>(file);
+    if (wavFile == nullptr) {
+        return false;
+    }
+    auto tag = wavFile->ID3v2Tag();
+    if (tag != nullptr) {
+        try {
+            jBuilder.setId3v2(*tag);
+        } catch (std::exception &e) {
+            LOGE("Unable to parse ID3v2 tag in %s: %s", name, e.what());
+        }
+    }
+    return true;
+}
+
 extern "C" JNIEXPORT jobject JNICALL
 Java_org_oxycblt_musikr_metadata_TagLibJNI_openNative(JNIEnv *env,
         jobject /* this */,
         jobject inputStream) {
+    const char *name = nullptr;
     try {
         JInputStream jStream {env, inputStream};
+        name = jStream.name();
         TagLib::FileRef fileRef {&jStream};
         if (fileRef.isNull()) {
-            LOGE("Error opening file");
-            return nullptr;
+            throw std::runtime_error("Invalid file");
         }
         TagLib::File *file = fileRef.file();
         JMetadataBuilder jBuilder {env};
+        jBuilder.setProperties(file->audioProperties());
 
-        if (auto *mpegFile = dynamic_cast<TagLib::MPEG::File *>(file)) {
+        // TODO: Make some type of composable logger so I don't
+        //  have to shoehorn this into the native code.
+        if (parseMpeg(name, file, jBuilder)) {
             jBuilder.setMimeType("audio/mpeg");
-            auto id3v1Tag = mpegFile->ID3v1Tag();
-            if (id3v1Tag != nullptr) {
-                jBuilder.setId3v1(*id3v1Tag);
-            }
-            auto id3v2Tag = mpegFile->ID3v2Tag();
-            if (id3v2Tag != nullptr) {
-                jBuilder.setId3v2(*id3v2Tag);
-            }
-        } else if (auto *mp4File = dynamic_cast<TagLib::MP4::File *>(file)) {
+        } else if (parseMp4(name, file, jBuilder)) {
             jBuilder.setMimeType("audio/mp4");
-            auto tag = mp4File->tag();
-            if (tag != nullptr) {
-                jBuilder.setMp4(*tag);
-            }
-        } else if (auto *flacFile = dynamic_cast<TagLib::FLAC::File *>(file)) {
+        } else if (parseFlac(name, file, jBuilder)) {
             jBuilder.setMimeType("audio/flac");
-            auto id3v1Tag = flacFile->ID3v1Tag();
-            if (id3v1Tag != nullptr) {
-                jBuilder.setId3v1(*id3v1Tag);
-            }
-            auto id3v2Tag = flacFile->ID3v2Tag();
-            if (id3v2Tag != nullptr) {
-                jBuilder.setId3v2(*id3v2Tag);
-            }
-            auto xiphComment = flacFile->xiphComment();
-            if (xiphComment != nullptr) {
-                jBuilder.setXiph(*xiphComment);
-            }
-            auto pics = flacFile->pictureList();
-            jBuilder.setFlacPictures(pics);
-        } else if (auto *opusFile = dynamic_cast<TagLib::Ogg::Opus::File *>(file)) {
+        } else if (parseOpus(name, file, jBuilder)) {
             jBuilder.setMimeType("audio/opus");
-            auto tag = opusFile->tag();
-            if (tag != nullptr) {
-                jBuilder.setXiph(*tag);
-            }
-        } else if (auto *vorbisFile =
-                dynamic_cast<TagLib::Ogg::Vorbis::File *>(file)) {
+        } else if (parseVorbis(name, file, jBuilder)) {
             jBuilder.setMimeType("audio/vorbis");
-            auto tag = vorbisFile->tag();
-            if (tag != nullptr) {
-                jBuilder.setXiph(*tag);
-            }
-        } else if (auto *wavFile = dynamic_cast<TagLib::RIFF::WAV::File *>(file)) {
+        } else if (parseWav(name, file, jBuilder)) {
             jBuilder.setMimeType("audio/wav");
-            auto tag = wavFile->ID3v2Tag();
-            if (tag != nullptr) {
-                jBuilder.setId3v2(*tag);
-            }
         } else {
-            // While taglib supports other formats, ExoPlayer does not. Ignore them.
-            LOGD("Unsupported file format");
+            LOGE("File format in %s is not supported", name);
             return nullptr;
         }
-
-        jBuilder.setProperties(file->audioProperties());
         return jBuilder.build();
-    } catch (std::runtime_error e) {
-        LOGD("Error opening file: %s", e.what());
+    } catch (std::exception &e) {
+        LOGE("Unable to parse metadata in %s: %s", name != nullptr ? name : "unknown file", e.what());
         return nullptr;
     }
-
 }

musikr/src/main/java/org/oxycblt/musikr/metadata/MetadataExtractor.kt

@@ -22,19 +22,20 @@ import android.os.ParcelFileDescriptor
 import java.io.FileInputStream
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
+import org.oxycblt.musikr.fs.DeviceFile
 
 internal interface MetadataExtractor {
-    suspend fun extract(fd: ParcelFileDescriptor): Metadata?
+    suspend fun extract(deviceFile: DeviceFile, fd: ParcelFileDescriptor): Metadata?
 
     companion object {
         fun new(): MetadataExtractor = MetadataExtractorImpl
     }
 }
 
 private object MetadataExtractorImpl : MetadataExtractor {
-    override suspend fun extract(fd: ParcelFileDescriptor) =
+    override suspend fun extract(deviceFile: DeviceFile, fd: ParcelFileDescriptor) =
         withContext(Dispatchers.IO) {
             val fis = FileInputStream(fd.fileDescriptor)
-            TagLibJNI.open(fis).also { fis.close() }
+            TagLibJNI.open(deviceFile, fis).also { fis.close() }
         }
 }

musikr/src/main/java/org/oxycblt/musikr/metadata/NativeInputStream.kt

@@ -21,10 +21,13 @@ package org.oxycblt.musikr.metadata
 import android.util.Log
 import java.io.FileInputStream
 import java.nio.ByteBuffer
+import org.oxycblt.musikr.fs.DeviceFile
 
-internal class NativeInputStream(fis: FileInputStream) {
+internal class NativeInputStream(private val deviceFile: DeviceFile, fis: FileInputStream) {
     private val channel = fis.channel
 
+    fun name() = requireNotNull(deviceFile.path.name)
+
     fun readBlock(length: Long): ByteArray? {
         try {
             val buffer = ByteBuffer.allocate(length.toInt())

musikr/src/main/java/org/oxycblt/musikr/metadata/TagLibJNI.kt

@@ -19,6 +19,7 @@
 package org.oxycblt.musikr.metadata
 
 import java.io.FileInputStream
+import org.oxycblt.musikr.fs.DeviceFile
 
 internal object TagLibJNI {
     init {
@@ -30,12 +31,12 @@ internal object TagLibJNI {
      *
      * Note: This method is blocking and should be handled as such if calling from a coroutine.
      */
-    fun open(fis: FileInputStream): Metadata? {
-        val inputStream = NativeInputStream(fis)
+    fun open(deviceFile: DeviceFile, fis: FileInputStream): Metadata? {
+        val inputStream = NativeInputStream(deviceFile, fis)
         val tag = openNative(inputStream)
         inputStream.close()
         return tag
     }
 
-    private external fun openNative(ioStream: NativeInputStream): Metadata?
+    private external fun openNative(inputStream: NativeInputStream): Metadata?
 }

musikr/src/main/java/org/oxycblt/musikr/pipeline/ExtractStep.kt

@@ -117,7 +117,7 @@ private class ExtractStepImpl(
             fds.mapNotNull { fileWith ->
                     wrap(fileWith.file) { _ ->
                         metadataExtractor
-                            .extract(fileWith.with)
+                            .extract(fileWith.file, fileWith.with)
                             ?.let { FileWith(fileWith.file, it) }
                             .also { withContext(Dispatchers.IO) { fileWith.with.close() } }
                     }