HI, could you send me this audio file (as attachment here, or via mail)? It helps me solve your problem;)

Marcin, you don't need the audio file, you can reproduce the bug by saving the same tags as one of the songs of that album (artist, title, album) in one of your MP3 files, import it, and play it (then it will try to download the cover art).

OK, thanks for your advice;)
Problem is a little bit wired, because every product of this artist doesn't work properly. I'm going to look closer at this bug.

Created attachment 273450 [details] [review]
proposed bug fix

After quite long investigation, I really don't know why amazon doesn't return valid cover for this artist :/
I prepared a patch, which detects 404 image - it's a little bit hacky, and if you've got a better idea to detect it (we might for example compare entire file, but IMO it's not necessary), let me know about it, I will fix it

Comment on attachment 273450 [details] [review]
proposed bug fix

Thanks for the patch Marcin!

As you already say, I agree it's a bit hacky, but it's better than nothing! Hopefully after a couple of reviews it will be ready to merge, so just be patient and persistent please :) Here goes:

+            if (IsAmazon404Image(path)) {
+                File.Delete (path);
+                return false;
+            }

When I titled the bug "should interpret as 404" it was just so it could be easily understood, that is, you shouldn't write the code to make someone think that there's a special HTTP code from Amazon which is a special "404" :) You could use simply ImageHasInvalidSize() (and write a comment inside that says that Amazon returns 1x1 images which are not acceptable to be shown in banshee).

But, the most important thing about reviewing this part of the code is its workflow: it is not acceptable that we save a file, and then after evaluating it, we delete it, because in the small time-window in which we do the evaluation, that file could be seen by other thread, and make banshee think that the artwork for that cover exists.

In fact, banshee's codebase is already careful about this in other cases, for example, look at this piece of code extracted from the SaveAtomically() method:

 // Save the file to a temporary path while downloading/copying,
 // so that nobody sees it and thinks it's ready for use before it is
 SafeUri tmp_uri = new SafeUri (String.Format ("{0}.part", path));
 try {
     Banshee.IO.StreamAssist.Save (from_stream, Banshee.IO.File.OpenWrite (tmp_uri, true));
     Banshee.IO.File.Move (tmp_uri, path_uri);
 } catch (Exception e) {
     Hyena.Log.Exception (e);
 }

This also can give you a hint of when you should be checking the image size: you should do it after Banshee.IO.StreamAssist.Save() and before IO.File.Move() is called.

+        public static bool IsAmazon404Image(string path)
+        {
+            using (BinaryReader binaryReader = new BinaryReader(File.OpenRead(path))) {
+                var gif_header = new byte[]{ 0x47, 0x49, 0x46 };

I'm thinking that there should be some library which we must be already using in Banshee, which can check the image size for us. We shouldn't write such a low level code. Maybe Gdk.Pixbuf can do this?

Thanks!

Created attachment 273494 [details] [review]
proposed bug fix

Thanks for your advices, I hope, I'm better banshee developer after your every comment;) I improved my patch, and probably it should works, but unfortunatelly, I can't compile it (and of course, check my changes). I've got an error:
Making all in Banshee.Services
  MCS   ../../../bin/Banshee.Services.dll
./Banshee.Metadata/MetadataServiceJob.cs(167,13): error CS0103: The name `Gdk' does not exist in the current context
I don't know why... I added gdk-sharp reference, and it still won't compile. Maybe you could give me a tip? 
Thanks ;)

The references of projects are added here: https://github.com/GNOME/banshee/blob/master/build/build.environment.mk

Created attachment 273500 [details] [review]
proposed bug fix 2

Thank you very much for your mentoring! I attach a patch, which (I hope) finally solves that problem. I modified signature of SaveAtomically method. IMO developer should know, whether saving process finished properly.

Ok, we're nearly there! we just need a fix wrt to references and some nitpicks:

> Subject: [PATCH] detecting nonexisting amazon audiofile's cover (bgo#690332)

Read our git commit guidelines please: you need a word that says the area of the code you're changing (in this case "MetadataService: " would do), and then later write a paragraph about the 'what' and the 'why' of the change.


+        public static bool ImageHasInvalidSize(SafeUri uri)

I'm guessing you meant to decorate this method as private, not public, right?
Also, you forgot to add a space between the method name and the parenthesis.


+            Gdk.Pixbuf.GetFileInfo (uri.AbsolutePath, out width, out height);
+            return width == 1 && height == 1;

Let's just return width =< 1 || height =< 1 here.


+                    if (!SaveAtomically (CoverArtSpec.GetPathForNewFile (Track.ArtworkId, best_file), Banshee.IO.File.OpenRead (new SafeUri (best_file))))
+                        return;

Check our HACKING file: we require braces for "if" clauses even if the "then" block is only one line.


 
-REF_BANSHEE_SERVICES = $(LINK_BANSHEE_CORE_DEPS) $(LINK_MONO_MEDIA_DEPS) $(LINK_LASTFM_DEPS) $(LINK_MUSICBRAINZ_DEPS)
+REF_BANSHEE_SERVICES = $(LINK_BANSHEE_CORE_DEPS) $(LINK_MONO_MEDIA_DEPS) $(LINK_LASTFM_DEPS) $(LINK_MUSICBRAINZ_DEPS) $(LINK_GTK)

This doesn't match to what you did in the .csproj file, because LINK_GTK links to Gtk and Gdk I guess. This is not good from an architecture point of view because we don't want a Core library to link to a GUI library (GDK is also kind of a UI library, but I don't see this one as important; let's say we develop a QT frontend in the future? we wouldn't want that frontend to link to a core library that links to GTK, maybe GDK is acceptable, but if not, replacing the minimal usage of Gdk.Pixbuf with something else would be seen as much easier than replacing GTK from a Core library ;) )

To do this maybe you'll need to modify some autotools bits, because I guess LINK_GDK var doesn't exist. I'm even thinking that maybe you need to deal with .pc files too? well, given that you have experience with bindings (gstreamermm) I'm guessing you will not have many problems to solve this. In any case you can also make a pull-request to gtk-sharp in case you find anything missing (the maintainers are friendly and responsive, and I'd help you ping them): https://github.com/mono/gtk-sharp/

Thanks!

Thanks for your comment, you're very patient for me, it's impressing!
I prepared pull-request: https://github.com/mono/gtk-sharp/pull/101
When it will be accepted, I'm going to prepare a new banshee's patch, and send it here.

Isn't System.Drawing maybe the better approach here (over Gdk)? Most parts of it are fully managed don't need libgdiplus AFAIK (not sure if this applies to this image size check). Ignore this comment if System.Drawing requires libgdiplus for this to work, as libgdiplus is not better over requiring the GDK library.

Hey Mirco!

(In reply to comment #11)
> Isn't System.Drawing maybe the better approach here (over Gdk)? Most parts of
> it are fully managed don't need libgdiplus AFAIK (not sure if this applies to
> this image size check). Ignore this comment if System.Drawing requires
> libgdiplus for this to work, as libgdiplus is not better over requiring the GDK
> library.

That's a nice suggestion. Marcin: can you confirm what Mirco suggests?

Mirco: BTW, what do you think about the gtk-sharp pull request?

Thanks to both!

That's what I found here: http://www.mono-project.com/Drawing
"Our implementation is a C# wrapper around the GDI+ C API (also called the "GDI+ Flat API")."
So doesn't matter, libgdiplus is also used in a System.Drawing implementation.

(In reply to comment #13)
> That's what I found here: http://www.mono-project.com/Drawing
> "Our implementation is a C# wrapper around the GDI+ C API (also called the
> "GDI+ Flat API")."
> So doesn't matter, libgdiplus is also used in a System.Drawing implementation.

Can you confirm that System.Drawing's API for getting the images's width and height is effectively p/invoking libgdiplus.so*? Meebey: or would this question not matter given that S.Drawing links to libgdiplus.so* anyway?

Disregard comment#14, Mirco just confirmed that Sys.D is no better than GDK because of this. We'll wait for Bertrand's feedback on the PR (and we'll probably need to wait for a new gtk# version to be able to commit the final fix for this bug then).

OK, so for now I suspend work around this bug, I've got a local branch with my changes, and I'll send here a patch when new gtk# version is comming (btw. is there some kind of subscription? I'm afraid that I forget about it, and mail notification about new version and with changelog would be helpful for me;)

Coming from the Gtk# pull request, an idea just came to mind:
We could use taglib-sharp to get the image dimensions: it supports images (GIF, JPEG, etc.), and it's already a dependency of the Banshee core.

I haven't really looked into it, so it's just a random idea at this point.

Banshee is not under active development anymore and had its last code changes more than three years ago. Its codebase has been archived.

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Please feel free to reopen this ticket (or rather transfer the project
to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the
responsibility for active development again.
See https://gitlab.gnome.org/Infrastructure/Infrastructure/issues/264 for more info.