search

erikas-taroza / audiotags

Read and write audio metadata in Flutter. Supports multiple formats.
https://pub.dev/packages/audiotags
MIT License
9 stars 4 forks source link

[Bug] Exception unhandled with certain audio files #7

Closed dannyglover closed 7 months ago

dannyglover commented 7 months ago

Hello again.

I get an unhandled exception when attempting to read audio tags from certain files via audioTags.read().

Here's a file that exhibits the issue: Download here

Log for flutter run -v

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Instance of 'FrbAnyhowException'
[        ] #0      FlutterRustBridgeBase._transformRust2DartMessage (package:flutter_rust_bridge/src/basic.dart:133:9)
[        ] #1      FlutterRustBridgeBase.executeNormal.<anonymous closure> (package:flutter_rust_bridge/src/basic.dart:70:51)
[        ] <asynchronous suspension>
[        ] #2      AudioTags.read (package:audiotags/src/audiotags.dart:12:14)
[        ] <asynchronous suspension>
[        ] #3      main (package: myapp/main.dart:57:31)
[        ] <asynchronous suspension>
erikas-taroza commented 7 months ago

I tried your file out. It seems like the Rust dependency doesn't support webp images in the metadata. I'll see if I can fix the issue myself, but it might be a feature request issue which will take some time.

dannyglover commented 7 months ago

I tried your file out. It seems like the Rust dependency doesn't support webp images in the metadata. I'll see if I can fix the issue myself, but it might be a feature request issue which will take some time.

Ah, gotcha. In that case, I'd be fine with it removing that image from the returned metadata, as a workaround.

Much better than it throwing an exception. I wasn't even aware of any of my music using webp images. I'd be totally fine with the aforementioned workaround. I will be providing an interface to edit the metadata anyway, and don't plan on supporting webp. Having it not return any image in the case of it being webp would at least enable me to tell the user the image format isn't supported.

If it's possible for you to detect the image format of course.

Cheers.

erikas-taroza commented 7 months ago

I tried your file out. It seems like the Rust dependency doesn't support webp images in the metadata. I'll see if I can fix the issue myself, but it might be a feature request issue which will take some time.

Ah, gotcha. In that case, I'd be fine with it removing that image from the returned metadata, as a workaround.

Much better than it throwing an exception. I wasn't even aware of any of my music using webp images. I'd be totally fine with the aforementioned workaround. I will be providing an interface to edit the metadata anyway, and don't plan on supporting webp. Having it not return any image in the case of it being webp would at least enable me to tell the user the image format isn't supported.

If it's possible for you to detect the image format of course.

Cheers.

Yeah, I will work on improving errors now that the codegen tool is able to support custom error types.

I'll get back to this as soon as I can.

dannyglover commented 7 months ago

I tried your file out. It seems like the Rust dependency doesn't support webp images in the metadata. I'll see if I can fix the issue myself, but it might be a feature request issue which will take some time.

Ah, gotcha. In that case, I'd be fine with it removing that image from the returned metadata, as a workaround. Much better than it throwing an exception. I wasn't even aware of any of my music using webp images. I'd be totally fine with the aforementioned workaround. I will be providing an interface to edit the metadata anyway, and don't plan on supporting webp. Having it not return any image in the case of it being webp would at least enable me to tell the user the image format isn't supported. If it's possible for you to detect the image format of course. Cheers.

Yeah, I will work on improving errors now that the codegen tool is able to support custom error types.

I'll get back to this as soon as I can.

Improving errors would be great. But in cases like these, I feel it'd be best to return all the metadata that is valid (/fine) and return nothing (empty string/array/list) for any metadata that is invalid along with it.

For this specific case, being unable to read any metadata from the file, just because it contains a picture in an unsupported format seems heavy handed.

So being able to get the rest of the metadata and just no picture would be a far better experience right now.

If the picture list is empty, then I can just show a placeholder and allow the user to set a jpg/png for that file to resolve it. Rather than being unable to even read the song title/album, all because of a bad picture.

See my point? I don't mean any of that in an argumentative tone or anything, just to be clear. It's just what I feel would be the best solution.

Adding an error exception that can be consumed by dart that can also allow me to know that "this songs picture is in an unsupported format", whilst still being able to continue execution of my program + being able to display the rest of the songs metadata would be preferable.

Let me know what you think. Thanks!

erikas-taroza commented 7 months ago

I completely agree with you but I need to clarify.

The error comes from the dependency, lofty. Lofty supports an Unknown mime type for images. However, it fails to read the metadata without considering using the unknown mime type. Maybe it doesn't know what webp is. I am not sure, but I plan on taking a look into the source and creating a PR for it.

With lofty, you can't skip reading the image if it is invalid. See here

dannyglover commented 7 months ago

I completely agree with you but I need to clarify.

The error comes from the dependency, lofty. Lofty supports an Unknown mime type for images. However, it fails to read the metadata without considering using the unknown mime type. Maybe it doesn't know what webp is. I am not sure, but I plan on taking a look into the source and creating a PR for it.

With lofty, you can't skip reading the image if it is invalid. See here

Gotcha. Makes sense. I had a quick browse and noticed this: https://github.com/erikas-taroza/audiotags/blob/ccbda921b8c2d21525c01365649ced03f534d92b/rust/src/picture.rs#L130

According to the linked issue on GitHub, they merged a fix for the issue raised in the line I linked above.

Perhaps the "Unknown" enum can be used now? I plan on getting my dev environment set up asap so I can help with this too, or at least try :)

erikas-taroza commented 7 months ago

Perhaps the "Unknown" enum can be used now?

Yes, that is the plan. However, the error occurs before being able to read the metadata. You need a TaggedFile to do anything with metadata but the error is thrown before that struct can be acquired. This is an issue with lofty that I will hopefully be able to fix.

dannyglover commented 7 months ago

Perhaps the "Unknown" enum can be used now?

Yes, that is the plan. However, the error occurs before being able to read the metadata. You need a TaggedFile to do anything with metadata but the error is thrown before that struct can be acquired. This is an issue with lofty that I will hopefully be able to fix.

Thanks for the explanation. Shall I go ahead and open an issue on lofys GitHub repo about this issue too? Or do you want to take a crack at it yourself first? I assume this is the repo: https://github.com/Serial-ATA/lofty-rs

erikas-taroza commented 7 months ago

Thanks for the explanation. Shall I go ahead and open an issue on lofys GitHub repo about this issue too? Or do you want to take a crack at it yourself first? I assume this is the repo: https://github.com/Serial-ATA/lofty-rs

You can create an issue so that the maintainer is aware. The repo you linked is the correct one. I plan to take a look regardless.

Let me know if you have any troubles setting up your environment.

dannyglover commented 7 months ago

Thanks for the explanation. Shall I go ahead and open an issue on lofys GitHub repo about this issue too? Or do you want to take a crack at it yourself first? I assume this is the repo: https://github.com/Serial-ATA/lofty-rs

You can create an issue so that the maintainer is aware. The repo you linked is the correct one. I plan to take a look regardless.

Let me know if you have any troubles setting up your environment.

I reported the issue to the maintainers of lofty and he fixed it already. The guy works fast (like you :) )

https://github.com/Serial-ATA/lofty-rs/issues/253

erikas-taroza commented 7 months ago

That's good! Saves me some time :) I'll update the dependency when the new version is released tomorrow. Thanks for creating the issues.

dannyglover commented 7 months ago

That's good! Saves me some time :) I'll update the dependency when the new version is released tomorrow. Thanks for creating the issues.

Excellent 👍 Will that include pushing an update to pub.dev? I'm curious to see if I can get through the entirety of my music library with the update.

Who knows, I might have more odd music files that bring up further issues, heh.

I'm hope to start on that pr to add more metadata fields tomorrow.

Have a nice weekend.

erikas-taroza commented 7 months ago

Excellent 👍 Will that include pushing an update to pub.dev? I'm curious to see if I can get through the entirety of my music library with the update.

Sure, I can upload a patch for you.

erikas-taroza commented 7 months ago

I have uploaded version 1.2.0 to pub.dev

dannyglover commented 7 months ago

Got the plugin updated at my end, bad news though. It is now failing on a file that didn't fail before:

flutter: working on file /home/danny/Downloads/Play - K-391.flac
[ +126 ms] [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Exception: unexpected arr length: expect 9 but see 7
[   +1 ms] #0      AudiotagsImpl._wire2api_tag (package:audiotags/src/bridge_generated.dart:136:7)
[        ] #1      FlutterRustBridgeBase._parseData (package:flutter_rust_bridge/src/basic.dart:147:22)
[        ] #2      FlutterRustBridgeBase._transformRust2DartMessage (package:flutter_rust_bridge/src/basic.dart:131:16)
[        ] #3      FlutterRustBridgeBase.executeNormal.<anonymous closure> (package:flutter_rust_bridge/src/basic.dart:70:51)
[        ] <asynchronous suspension>
[        ] #4      AudioTags.read (package:audiotags/src/audiotags.dart:20:14)
[        ] <asynchronous suspension>
[        ] #5      main (package:play_the_music_player/main.dart:81:31)
[        ] <asynchronous suspension>

File: here

The file in question was working prior to the update.

I'm going to see if this happens with lofty-0.16 using their test suite. I'll report back.

Edit: It works fine using lofty-rs 0.16:

Running `target/debug/examples/tag_reader '/home/danny/Downloads/Play - K-391.flac'`
--- Tag Information ---
Title: Play
Artist: K-391
Album: Play
Genre: Électronique
Album Artist: K-391, Alan Walker, Martin Tungevaag, Mangoo
--- Audio Properties ---
Bitrate (Audio): 1706
Bitrate (Overall): 1710
Sample Rate: 44100
Bit depth: 24
Channels: 2
Duration: 02:47
erikas-taroza commented 7 months ago

Got the plugin updated at my end, bad news though. It is now failing on a file that didn't fail before:

flutter: working on file /home/danny/Downloads/Play - K-391.flac
[ +126 ms] [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Exception: unexpected arr length: expect 9 but see 7
[   +1 ms] #0      AudiotagsImpl._wire2api_tag (package:audiotags/src/bridge_generated.dart:136:7)
[        ] #1      FlutterRustBridgeBase._parseData (package:flutter_rust_bridge/src/basic.dart:147:22)
[        ] #2      FlutterRustBridgeBase._transformRust2DartMessage (package:flutter_rust_bridge/src/basic.dart:131:16)
[        ] #3      FlutterRustBridgeBase.executeNormal.<anonymous closure> (package:flutter_rust_bridge/src/basic.dart:70:51)
[        ] <asynchronous suspension>
[        ] #4      AudioTags.read (package:audiotags/src/audiotags.dart:20:14)
[        ] <asynchronous suspension>
[        ] #5      main (package:play_the_music_player/main.dart:81:31)
[        ] <asynchronous suspension>

File: here

The file in question was working prior to the update.

I'm going to see if this happens with lofty-0.16 using their test suite. I'll report back.

Edit: It works fine using lofty-rs 0.16:

Running `target/debug/examples/tag_reader '/home/danny/Downloads/Play - K-391.flac'`
--- Tag Information ---
Title: Play
Artist: K-391
Album: Play
Genre: Électronique
Album Artist: K-391, Alan Walker, Martin Tungevaag, Mangoo
--- Audio Properties ---
Bitrate (Audio): 1706
Bitrate (Overall): 1710
Sample Rate: 44100
Bit depth: 24
Channels: 2
Duration: 02:47

I just tested this. Works fine on my end :thinking: Maybe its a cache issue?

dannyglover commented 7 months ago

Got the plugin updated at my end, bad news though. It is now failing on a file that didn't fail before:

flutter: working on file /home/danny/Downloads/Play - K-391.flac
[ +126 ms] [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Exception: unexpected arr length: expect 9 but see 7
[   +1 ms] #0      AudiotagsImpl._wire2api_tag (package:audiotags/src/bridge_generated.dart:136:7)
[        ] #1      FlutterRustBridgeBase._parseData (package:flutter_rust_bridge/src/basic.dart:147:22)
[        ] #2      FlutterRustBridgeBase._transformRust2DartMessage (package:flutter_rust_bridge/src/basic.dart:131:16)
[        ] #3      FlutterRustBridgeBase.executeNormal.<anonymous closure> (package:flutter_rust_bridge/src/basic.dart:70:51)
[        ] <asynchronous suspension>
[        ] #4      AudioTags.read (package:audiotags/src/audiotags.dart:20:14)
[        ] <asynchronous suspension>
[        ] #5      main (package:play_the_music_player/main.dart:81:31)
[        ] <asynchronous suspension>

File: here The file in question was working prior to the update. I'm going to see if this happens with lofty-0.16 using their test suite. I'll report back. Edit: It works fine using lofty-rs 0.16:

Running `target/debug/examples/tag_reader '/home/danny/Downloads/Play - K-391.flac'`
--- Tag Information ---
Title: Play
Artist: K-391
Album: Play
Genre: Électronique
Album Artist: K-391, Alan Walker, Martin Tungevaag, Mangoo
--- Audio Properties ---
Bitrate (Audio): 1706
Bitrate (Overall): 1710
Sample Rate: 44100
Bit depth: 24
Channels: 2
Duration: 02:47

I just tested this. Works fine on my end 🤔 Maybe its a cache issue?

I tried a flutter clean, hmm.. I'll try removing the plugin via flutter pub remove audiotags, then do a flutter clean then add the plugin again.

I'll report back :)

erikas-taroza commented 7 months ago

I tried a flutter clean, hmm.. I'll try removing the plugin via flutter pub remove audiotags, then do a flutter clean then add the plugin again.

Ok. By the way, I tested on the example project.

dannyglover commented 7 months ago

No dice so far. The issue happens with any audio file currently.

I tried flutter clean, flutter pub clean and so on. I haven't changed my tag handling code between the old version and the new, but to rule out any issues, I'll try it on your example project and report back.

dannyglover commented 7 months ago

Ok, I can reproduce the issue with your example code.

If you leave the pubspec.yaml definition in your example code at the default setting (I cloned your repo) for audiotags to use the local path like so:

audiotags: 
    path: ../

It reads the tags fine. However, if you set it to use the pub.dev version like so:

audiotags: ^1.2.0

It crashes and burns with the same error I posted above.

erikas-taroza commented 7 months ago

Ok, I can reproduce the issue with your example code.

If you leave the pubspec.yaml definition for audiotags to use the local path like so:

audiotags: 
    path: ../

It reads the tags fine. However, if you set it to use the pub.dev version like so:

audiotags: ^1.2.0

It crashes and burns with the same error I posted above.

Ah found the issue. I forgot to update the .pubignore when I changed how the binaries were downloaded.

dannyglover commented 7 months ago

Ok, I can reproduce the issue with your example code. If you leave the pubspec.yaml definition for audiotags to use the local path like so:

audiotags: 
    path: ../

It reads the tags fine. However, if you set it to use the pub.dev version like so:

audiotags: ^1.2.0

It crashes and burns with the same error I posted above.

Ah found the issue. I forgot to update the .pubignore when I changed how the binaries were downloaded.

Excellent. Thanks bud.

PS: I was wondering how to use local versions of plugins (i.e. from source), so this was a useful exercise. I didn't get around to working on adding those new tag fields today (been sick), but hopefully this week. :)

erikas-taroza commented 7 months ago

Excellent. Thanks bud.

PS: I was wondering how to use local versions of plugins (i.e. from source), so this was a useful exercise. I didn't get around to working on adding those new tag fields today (been sick), but hopefully this week. :)

No problem :) Take care

erikas-taroza commented 7 months ago

Just created a new version 1.2.1. Tested it in the example and it works.

dannyglover commented 7 months ago

Just created a new version 1.2.1. Tested it in the example and it works.

Ok, final update on this issue. I have great news this time. I pulled the latest from pub.dev and I am now able to get through my entire library of music (996 songs) without any errors.

Great stuff bud :)

erikas-taroza commented 7 months ago

Just created a new version 1.2.1. Tested it in the example and it works.

Ok, final update on this issue. I have great news this time. I pulled the latest from pub.dev and I am now able to get through my entire library of music (996 songs) without any errors.

Great stuff bud :)

Glad to hear it works :)