From d49286981c0ba1e32c8504e066b200bf22513c9f Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 20 Jan 2025 11:26:41 -0700 Subject: [PATCH] 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 | 9 +- musikr/src/main/cpp/JInputStream.h | 1 + musikr/src/main/cpp/JStringRef.cpp | 10 + musikr/src/main/cpp/JStringRef.h | 4 + musikr/src/main/cpp/taglib_jni.cpp | 200 +++++++++++++----- .../musikr/metadata/MetadataExtractor.kt | 7 +- .../musikr/metadata/NativeInputStream.kt | 5 +- .../org/oxycblt/musikr/metadata/TagLibJNI.kt | 7 +- .../oxycblt/musikr/pipeline/ExtractStep.kt | 2 +- 9 files changed, 181 insertions(+), 64 deletions(-) diff --git a/musikr/src/main/cpp/JInputStream.cpp b/musikr/src/main/cpp/JInputStream.cpp index 14693d394..3ea9c80c7 100644 --- a/musikr/src/main/cpp/JInputStream.cpp +++ b/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(env->CallObjectMethod( + jInputStream, jInputStreamNameMethod)) }; + return jName.copy().toCString(); } TagLib::ByteVector JInputStream::readBlock(size_t length) { diff --git a/musikr/src/main/cpp/JInputStream.h b/musikr/src/main/cpp/JInputStream.h index 084f9dc31..ce3399d4d 100644 --- a/musikr/src/main/cpp/JInputStream.h +++ b/musikr/src/main/cpp/JInputStream.h @@ -115,6 +115,7 @@ public: private: JNIEnv *env; jobject jInputStream; + jmethodID jInputStreamNameMethod; jmethodID jInputStreamReadBlockMethod; jmethodID jInputStreamIsOpenMethod; jmethodID jInputStreamSeekFromBeginningMethod; diff --git a/musikr/src/main/cpp/JStringRef.cpp b/musikr/src/main/cpp/JStringRef.cpp index e6b1bf71d..c91b863d5 100644 --- a/musikr/src/main/cpp/JStringRef.cpp +++ b/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; } diff --git a/musikr/src/main/cpp/JStringRef.h b/musikr/src/main/cpp/JStringRef.h index ffe4ab8ad..89fcb046d 100644 --- a/musikr/src/main/cpp/JStringRef.h +++ b/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 @@ public: JStringRef& operator=(const JStringRef&) = delete; + TagLib::String copy(); + jstring& operator*(); private: diff --git a/musikr/src/main/cpp/taglib_jni.cpp b/musikr/src/main/cpp/taglib_jni.cpp index 2bc040ea0..c2679cdff 100644 --- a/musikr/src/main/cpp/taglib_jni.cpp +++ b/musikr/src/main/cpp/taglib_jni.cpp @@ -30,82 +30,174 @@ #include "taglib/vorbisfile.h" #include "taglib/wavfile.h" +std::unique_ptr openFile(JNIEnv *env, jobject inputStream) { +} + +bool parseMpeg(const char *name, TagLib::File *file, + JMetadataBuilder &jBuilder) { + auto *mpegFile = dynamic_cast(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(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(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(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(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(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(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(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(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(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(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(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; } - } diff --git a/musikr/src/main/java/org/oxycblt/musikr/metadata/MetadataExtractor.kt b/musikr/src/main/java/org/oxycblt/musikr/metadata/MetadataExtractor.kt index 84478adbd..0b0cfc48d 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/metadata/MetadataExtractor.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/metadata/MetadataExtractor.kt @@ -22,9 +22,10 @@ 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 @@ -32,9 +33,9 @@ internal interface MetadataExtractor { } 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() } } } diff --git a/musikr/src/main/java/org/oxycblt/musikr/metadata/NativeInputStream.kt b/musikr/src/main/java/org/oxycblt/musikr/metadata/NativeInputStream.kt index e5bb2fb69..1f5a01a99 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/metadata/NativeInputStream.kt +++ b/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()) diff --git a/musikr/src/main/java/org/oxycblt/musikr/metadata/TagLibJNI.kt b/musikr/src/main/java/org/oxycblt/musikr/metadata/TagLibJNI.kt index ed440c20a..d5105f3a8 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/metadata/TagLibJNI.kt +++ b/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? } diff --git a/musikr/src/main/java/org/oxycblt/musikr/pipeline/ExtractStep.kt b/musikr/src/main/java/org/oxycblt/musikr/pipeline/ExtractStep.kt index ad34d8f82..bcf1f9fd5 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/pipeline/ExtractStep.kt +++ b/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() } } }