Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_embedart.py: EmbedartCliTest.test_accept_similar_art fails #4836

Closed
doronbehar opened this issue Jun 30, 2023 · 7 comments · Fixed by #4839
Closed

test_embedart.py: EmbedartCliTest.test_accept_similar_art fails #4836

doronbehar opened this issue Jun 30, 2023 · 7 comments · Fixed by #4839
Labels
bug bugs that are confirmed and actionable

Comments

@doronbehar
Copy link
Contributor

Problem

Here on NixOS, we continuously rebuild all packages that their dependencies have been changed. Recently, beets have been failing to build because it's tests have failed:

=================================== FAILURES ===================================
___________________ EmbedartCliTest.test_accept_similar_art ____________________

args = (<test.test_embedart.EmbedartCliTest testMethod=test_accept_similar_art>,)
kwargs = {}

    def wrapper(*args, **kwargs):
        if not ArtResizer.shared.can_compare:
            raise unittest.SkipTest("compare not available")
        else:
>           return test(*args, **kwargs)

test/test_embedart.py:38: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
test/test_embedart.py:170: in test_accept_similar_art
    self.assertEqual(mediafile.images[0].data, self.image_data,
E   AssertionError: b'\xf[35 chars]01\x00\x00\x01\x00\x01\x00\x00\xff\xdb\x00C\x0[34652 chars]\xd9' != b'\xf[35 chars]01\x01\x00H\x00H\x00\x00\xff\xdb\x00C\x00\x03\[240374 chars]\xd9' : Image written is not /build/source/test/rsrc/abbey-similar.jpg

Full build log is available here:

https://hydra.nixos.org/build/226088427/nixlog/1

Setup

  • OS: NixOS
  • Python version: 3.10.11
  • beets version: 1.6.0
  • Turning off plugins made problem go away (yes/no): Irrelevant
doronbehar added a commit to doronbehar/nixpkgs that referenced this issue Jun 30, 2023
doronbehar added a commit to doronbehar/nixpkgs that referenced this issue Jun 30, 2023
iquerejeta pushed a commit to iquerejeta/nixpkgs that referenced this issue Jun 30, 2023
@Scrumplex
Copy link
Contributor

This was introduced by an imagemagick update (NixOS/nixpkgs#240476 NixOS/nixpkgs@e033449).

git bisect log
# bad: [4bc72cae107788bf3f24f30db2e2f685c9298dc9] Merge pull request #240491 from r-ryantm/auto-update/gcompris
# good: [50cd94e6230b7b7e2b8e7ba2be17f2931a6c82e7] Merge pull request #240428 from r-ryantm/auto-update/hcloud
git bisect start 'nixos-unstable' 'nixos-unstable~100'
# good: [13ab4547dc02e63589607c5cf1d3e2b18b0f5b64] Merge pull request #240384 from hraban/sbcl/2.3.6
git bisect good 13ab4547dc02e63589607c5cf1d3e2b18b0f5b64
# good: [3dadeec9a92e005ee4d0e5bbea725c250819ccaa] Merge pull request #240487 from chvp/upd/hookshot
git bisect good 3dadeec9a92e005ee4d0e5bbea725c250819ccaa
# bad: [aecd4e5de22dedd36c7d1544affe1e4d92d9610c] Merge pull request #240461 from aaronjheng/credential-detector
git bisect bad aecd4e5de22dedd36c7d1544affe1e4d92d9610c
# bad: [2d355cacaeb36af1258554496fec354f225c15a7] Merge pull request #240218 from NixOS/pr/mplhep_init
git bisect bad 2d355cacaeb36af1258554496fec354f225c15a7
# good: [3791e8908eca998d9ccb805f9b473e740e4ab14e] Merge pull request #240482 from natsukium/fzf-fish/update
git bisect good 3791e8908eca998d9ccb805f9b473e740e4ab14e
# good: [81bdbd2525a52200af0540eb3ca01d0dd73ef120] Merge pull request #240225 from leona-ya/paperless-1-16-5
git bisect good 81bdbd2525a52200af0540eb3ca01d0dd73ef120
# good: [560a07f5070b390103fbb3eb8ae12ed1b2955bf8] python310Packages.mplhep: init at 0.3.28
git bisect good 560a07f5070b390103fbb3eb8ae12ed1b2955bf8
# bad: [0431f99716658d09864cf1b6dac7ba23a6ddfc27] Merge pull request #240476 from dotlambda/imagemagick-7.1.1-12
git bisect bad 0431f99716658d09864cf1b6dac7ba23a6ddfc27
# bad: [e0334495f78fd862cbb6985b25b41dd197bb462c] imagemagick: 7.1.1-11 -> 7.1.1-12
git bisect bad e0334495f78fd862cbb6985b25b41dd197bb462c
# first bad commit: [e0334495f78fd862cbb6985b25b41dd197bb462c] imagemagick: 7.1.1-11 -> 7.1.1-12

mitchmindtree pushed a commit to mitchmindtree/nixpkgs that referenced this issue Jul 2, 2023
mitchmindtree pushed a commit to mitchmindtree/nixpkgs that referenced this issue Jul 2, 2023
github-actions bot pushed a commit to NixOS/nixpkgs that referenced this issue Jul 2, 2023
Reported here: beetbox/beets#4836

(cherry picked from commit 04ae8ff)
@sampsyo sampsyo added the bug bugs that are confirmed and actionable label Jul 3, 2023
@sampsyo
Copy link
Member

sampsyo commented Jul 3, 2023

Thanks for the detailed report! At a glance, it seems like the problem is that ImageMagick (i.e., magick compare) used to say "yes" to our similarity query in this test and now it says "no." That's too bad, and it's a little tricky to fix without a local way to reproduce it… has anybody reading this (perhaps using the above-indicated version of ImageMagick) been able to see this happening locally?

If so, doing some debugging in the compare method in artresizer.py could confirm this hypothesis. If that's what's going on, maybe we just need to change the test file so the similarity comparison triggers correctly again.

@Scrumplex
Copy link
Contributor

Yep. It looks like this is a problem with ImageMagick. I ran the comparison done by beets (convert <beets-src>/test/rsrc/abbey-similar.jpg /tmp/nix-shell.kbVep7/tmpl0ecssu_.jpg -colorspace gray MIFF:- | compare -metric PHASH - null:) with the two ImageMagick versions in question:

ImageMagick 7.1.1-12 Q16-HDRI x86_64 b3f8ed7a7:20230625 https://imagemagick.org
produces 136.198

ImageMagick 7.1.1-11 Q16-HDRI x86_64 f04a7eb33:20230528 https://imagemagick.org
produces 11.7978

@Scrumplex
Copy link
Contributor

I have bisected this behavior in ImageMagick:

# bad: [a09d8dd5e3a92362cf70c184670b23163587e6f8] release
# good: [11ffa6eb4548644a718158daa286295ed3174054] release
git bisect start '7.1.1-12' '7.1.1-11'
# bad: [195a19168f8dfbfedc21b20a1ca3515bac96f524] latest CSS
git bisect bad 195a19168f8dfbfedc21b20a1ca3515bac96f524
# bad: [cd6b43771b82392decefecadc86a9ba6fd30cad3] reject invalid BMP image @ https://github.com/ImageMagick/ImageMagick/issues/6393
git bisect bad cd6b43771b82392decefecadc86a9ba6fd30cad3
# bad: [3f9df4fd698ca93b304dee4691d7f98e1a99ffc4] Changed options for heif build.
git bisect bad 3f9df4fd698ca93b304dee4691d7f98e1a99ffc4
# bad: [f45cb56383bda833708f08d6f8a580c833ffd1c9] default colorspaces are xyY and HSB
git bisect bad f45cb56383bda833708f08d6f8a580c833ffd1c9
# good: [3c43475bb5dc7eec9af3babc789bf8bf65677e90] beta release
git bisect good 3c43475bb5dc7eec9af3babc789bf8bf65677e90
# first bad commit: [f45cb56383bda833708f08d6f8a580c833ffd1c9] default colorspaces are xyY and HSB

The offending commit:

commit f45cb56383bda833708f08d6f8a580c833ffd1c9
Author: Cristy <urban-warrior@imagemagick.org>
Date:   Mon May 29 17:32:01 2023 -0400

    default colorspaces are xyY and HSB

diff --git a/MagickCore/statistic.c b/MagickCore/statistic.c
index 29f5e99b5..13b1592dc 100644
--- a/MagickCore/statistic.c
+++ b/MagickCore/statistic.c
@@ -1752,7 +1752,7 @@ MagickExport ChannelPerceptualHash *GetImagePerceptualHash(const Image *image,
   if (artifact != NULL)
     colorspaces=AcquireString(artifact);
   else
-    colorspaces=AcquireString("sRGB,HCLp");
+    colorspaces=AcquireString("xyY,HSB");
   perceptual_hash[0].number_colorspaces=0;
   perceptual_hash[0].number_channels=0;
   q=colorspaces;

Not sure if this is a bug or intended behavior.

@Scrumplex
Copy link
Contributor

Scrumplex commented Jul 3, 2023

A potential workaround is adding -define phash:colorspaces=sRGB,HCLp to the compare command. This restores the previous behavior of returning 11.7978

@sampsyo
Copy link
Member

sampsyo commented Jul 5, 2023

Wow; thank you for diagnosing this! I propose we just add that flag, since I confess I don't understand the nuances that made the ImageMagick folks prefer that default, so I have no grounds to believe it will switch back.

@Scrumplex
Copy link
Contributor

Great! I will prepare a PR in a moment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs that are confirmed and actionable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants