Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#33092 closed defect (fixed)

ffmpeg/imagemagick features need feature checks

Reported by: mjo Owned by:
Priority: blocker Milestone: sage-9.5
Component: packages: optional Keywords:
Cc: slabbe Merged in:
Authors: Sébastien Labbé Reviewers: Julian Rüth, Michael Orlitzky
Report Upstream: N/A Work issues:
Branch: 1678c7f (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mjo)

We have optional tests that make use of these packages, for example:

sage: a.show(format="webm", iterations=1)  # optional -- ffmpeg
sage: with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x14\x00' in f.read())  # optional -- ImageMagick

These tests are run whenever the corresponding "feature" is available, but the feature checks look only for the convert and ffmpeg programs. Both imagemagick and ffmpeg can be built without support for (say) webm files, making the tests above fail. To avoid spurious failures, the features should test for the necessary file format support, likely in the is_functional() method.

Attachments (2)

ffmpeg-imagemagick.log (140.0 KB) - added by mjo 5 months ago.
ffmpeg.log (36.8 KB) - added by mjo 5 months ago.

Download all attachments as: .zip

Change History (90)

comment:1 Changed 5 months ago by mkoeppe

  • Cc slabbe added

I don't think we need to tighten spkg-configure.m4 -- this does not influence the ffmpeg optional tag at all.

These tags are added by runtime Feature checks, not via spkg-configure.m4

comment:2 Changed 5 months ago by mkoeppe

(this code is from #32174)

comment:3 Changed 5 months ago by mjo

  • Description modified (diff)

comment:4 Changed 5 months ago by mkoeppe

If these doctests are the only ways that we "need" these features, perhaps it would be enough to just mark them as "not tested"?

comment:5 follow-up: Changed 5 months ago by slabbe

There are like 79 optional imagemagick doctests and 19 optional ffmpeg doctests:

$ git grep "optional -* ImageMagick" | wc
     76     678    7649
$ git grep "optional -* imagemagick" | wc
      3      23     305
$ git grep "optional -* ffmpeg" | wc
     19     151    1895

We can mark two of them as "not tested" if they do not work with all versions of ffmpeg/imagemagick. But I still think it is a good thing to test that the remaining 79 + 19 - 2 doctests still work on all versions of ffmpeg.

My opinion is that either we expect doctests to work and we test them or we expect doctests not to work and we do not test them.

comment:6 in reply to: ↑ 5 Changed 5 months ago by mkoeppe

Replying to slabbe:

We can mark two of them as "not tested" if they do not work with all versions of ffmpeg/imagemagick. But I still think it is a good thing to test that the remaining 79 + 19 - 2 doctests still work on all versions of ffmpeg.

I agree, I meant these 2 failing ones only.

comment:7 Changed 5 months ago by mjo

Those two were only examples. If I turn off all the features that Gentoo lets me turn off on my desktop (some are required by other packages),

sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/combinat/crystals/mv_polytopes.py  # 1 doctest failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/combinat/tiling.py  # 8 doctests failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/combinat/words/paths.py  # 7 doctests failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/plot/animate.py  # 66 doctests failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/plot/plot3d/tachyon.py  # 3 doctests failed

comment:8 follow-up: Changed 5 months ago by slabbe

I see. Can you provide the log of the failing doctests here or somewhere?

Changed 5 months ago by mjo

comment:9 in reply to: ↑ 8 Changed 5 months ago by mjo

Replying to slabbe:

I see. Can you provide the log of the failing doctests here or somewhere?

Sure, I just attached it.

comment:10 Changed 5 months ago by slabbe

Thank you for the log.

I see the error message convert convert: No decode delegate for this image format (00000000.png). Can you provide what is the output of convert -list Format on this machine? I wonder if the output of such command could be used to check that the feature works in the feature code?

Also I see:

subprocess.CalledProcessError: Command 'cd "/home/mjo/.sage/temp/gantu/27410/dir_787qb00e/"; sage-native-execute convert -dispose Background -delay 35 -loop 3 *.png "/home/mjo/.sage/temp/gantu/27410/dir_m_q6dl3q/my_animation.gif"' returned non-zero exit status 1.

which does not say much about the error. I think the check_call(cmd, shell=True) should be replaced in the method _gif_from_imagemagick by something which returns more info when it fails. I did something like that in the ticket #20343 (see method def pdf). We could do the same here.

Last edited 5 months ago by slabbe (previous) (diff)

comment:11 Changed 5 months ago by mjo

It looks like most of the disabled formats are absent from the list, but SVG and SVGZ (also disabled) appear with mode ---:

$ convert -list Format
   Format L  Mode  Description
--------------------------------------------------------------------------------
      3FR S  r--  Hasselblad Photo RAW
     8BIM P  rw-  Photoshop resource format
 8BIMTEXT P  rw-  Photoshop resource text format
8BIMWTEXT P  rw-  Photoshop resource wide text format
     APP1 P  rw-  Raw application information
 APP1JPEG P  rw-  Raw JPEG binary data
      ART S  rw-  PFS: 1st Publisher
      ARW S  r--  Sony Alpha DSLR RAW
      AVS U  rw+  AVS X image
        B S  rw+  Raw blue samples
      BMP P  rw-  Microsoft Windows bitmap image
     BMP2 P  -w-  Microsoft Windows bitmap image v2
     BMP3 P  -w-  Microsoft Windows bitmap image v3
      BRF S  -w-  BRF ASCII Braille format
        C S  rw+  Raw cyan samples
    CACHE U  ---  Magick Persistent Cache image format
     CALS S  rw-  Continuous Acquisition and Life-cycle Support Type 1 image
            Specified in MIL-R-28002 and MIL-PRF-28002
  CAPTION P  r--  Image caption
      CIN S  rw-  Cineon Image File
     CMYK S  rw+  Raw cyan, magenta, yellow, and black samples
    CMYKA S  rw+  Raw cyan, magenta, yellow, black, and opacity samples
      CR2 S  r--  Canon Photo RAW
      CRW S  r--  Canon Photo RAW
      CUR S  r--  Microsoft Cursor Icon
      CUT S  r--  DR Halo
      DCM S  r--  Digital Imaging and Communications in Medicine image
            See http://medical.nema.org/ for information on DICOM.
      DCR S  r--  Kodak Photo RAW
      DCX S  rw+  ZSoft IBM PC multi-page Paintbrush
      DNG S  r--  Adobe Digital Negative
      DPX P  rw-  SMPTE 268M-2003 (DPX 2.0)
            See http://www.smtpe.org/ for information on DPX.
     EPDF P  rw-  Encapsulated Portable Document Format
      EPI P  rw-  Adobe Encapsulated PostScript Interchange format
      EPS P  rw-  Adobe Encapsulated PostScript
     EPS2 P  -w-  Adobe Level II Encapsulated PostScript
     EPS3 P  -w+  Adobe Level III Encapsulated PostScript
     EPSF P  rw-  Adobe Encapsulated PostScript
     EPSI P  rw-  Adobe Encapsulated PostScript Interchange format
      ERF S  r--  Epson RAW Format
     EXIF P  rw-  Exif digital camera binary data
      FAX P  rw+  Group 3 FAX (Not TIFF Group3 FAX!)
     FITS S  rw-  Flexible Image Transport System
  FRACTAL S  r--  Plasma fractal image
        G S  rw+  Raw green samples
      GIF P  rw+  CompuServe graphics interchange format (version 89a)
    GIF87 P  rw-  CompuServe graphics interchange format (version 87a)
 GRADIENT P  r--  Gradual passing from one shade to another
     GRAY S  rw+  Raw gray samples
    GRAYA S  rw+  Raw gray samples + alpha
HISTOGRAM P  -w-  Histogram of the image
      HRZ S  r--  HRZ: Slow scan TV
     HTML S  -w-  Hypertext Markup Language and a client-side image map
      ICB S  rw+  Truevision Targa image
      ICC P  rw-  ICC Color Profile
      ICM P  rw-  ICC Color Profile
      ICO S  r--  Microsoft Icon
     ICON S  r--  Microsoft Icon
 IDENTITY P  r--  Hald CLUT identity image
    IMAGE P  r--  GraphicsMagick Embedded Image
     INFO S  -w+  Image descriptive information and statistics
     IPTC P  rw-  IPTC Newsphoto
 IPTCTEXT P  rw-  IPTC Newsphoto text format
IPTCWTEXT P  rw-  IPTC Newsphoto text format
   ISOBRL S  -w-  ISO/TR 11548-1 format
  ISOBRL6 S  -w-  ISO/TR 11548-1 format 6dot
        K S  rw+  Raw black samples
      K25 S  r--  Kodak Photo RAW
      KDC S  r--  Kodak Photo RAW
    LABEL P  r--  Image label
        M S  rw+  Raw magenta samples
      M2V S  -w+  MPEG Video Stream
      MAC S  r--  Mac Paint
      MAP U  rw-  Colormap intensities and indices
      MAT S  rw+  MATLAB Level 4.0-6.0 image formats
    MATTE S  -w+  MATTE raw opacity format
      MEF S  r--  Mamiya Photo RAW
     MIFF P  rw+  Magick Image File Format (GraphicsMagick 1.3.36)
     MONO S  rw-  Bi-level bitmap in least-significant-byte first order
      MPC U  rw+  Magick Persistent Cache image format
     MPEG S  -w+  MPEG Video Stream
      MPG S  -w+  MPEG Video Stream
      MRW S  r--  Minolta Photo RAW
      MTV U  rw+  MTV Raytracing image format
      MVG S  rw-  Magick Vector Graphics
      NEF S  r--  Nikon Electronic Format
     NULL P  rw-  Constant image of uniform color
        O S  rw+  Raw opacity samples
      ORF S  r--  Olympus Photo RAW
      OTB S  rw-  On-the-air bitmap
       P7 S  rw+  Xv thumbnail format
      PAL S  rw-  16bit/pixel interleaved YUV
     PALM U  r--  Palm pixmap
      PAM P  rw+  Portable Arbitrary Map format
      PBM P  rw+  Portable bitmap format (black/white)
      PCD S  rw-  Photo CD
     PCDS S  rw-  Photo CD
      PCL S  -w+  Page Control Language
      PCT S  rw-  Apple Macintosh QuickDraw/PICT
      PCX S  rw-  ZSoft IBM PC Paintbrush
      PDB U  rw+  Palm Database ImageViewer Format
      PDF P  rw+  Portable Document Format
      PEF S  r--  Pentax Electronic File
      PFA P  ---  Postscript Type 1 font (ASCII)
      PFB P  ---  Postscript Type 1 font (binary)
      PGM P  rw+  Portable graymap format (gray scale)
    PICON S  rw-  Personal Icon
     PICT S  rw-  Apple Macintosh QuickDraw/PICT
      PIX S  r--  Alias/Wavefront RLE image format
   PLASMA S  r--  Plasma fractal image
      PNM P  rw+  Portable anymap
      PPM P  rw+  Portable pixmap format (color)
  PREVIEW S  -w-  Show a preview an image enhancement, effect, or f/x
       PS P  rw+  Adobe PostScript
      PS2 P  -w+  Adobe Level II PostScript
      PS3 P  -w+  Adobe Level III PostScript
      PWP U  r--  Seattle Film Works
        R S  rw+  Raw red samples
      RAF S  r--  Fuji Photo RAW
      RAS S  rw+  SUN Rasterfile
      RGB S  rw+  Raw red, green, and blue samples
     RGBA S  rw+  Raw red, green, blue, and matte samples
      RLA U  r--  Alias/Wavefront image
      RLE U  r--  Utah Run length encoded image
      SCT U  r--  Scitex HandShake
      SFW U  r--  Seattle Film Works
      SGI S  rw-  Irix RGB image
    SHTML S  -w-  Hypertext Markup Language and a client-side image map
      SR2 S  r--  Sony Photo RAW
      SRF S  r--  Sony Photo RAW
  STEGANO S  r--  Steganographic image
      SUN S  rw+  SUN Rasterfile
      SVG S  ---  Scalable Vector Graphics
     SVGZ S  ---  Scalable Vector Graphics (ZIP compressed)
     TEXT S  rw+  ASCII Text
      TGA S  rw+  Truevision Targa image
     TILE P  r--  Tile image with a texture
            Use the syntax "-size WIDTHxHEIGHT TILE:imagename" to tile the
            specified tile image over a canvas image of size WIDTHxHEIGHT.
      TIM S  r--  PSX TIM
    TOPOL S  r--  TOPOL X Image
      TTF P  ---  TrueType font
      TXT S  rw+  ASCII Text
     UBRL S  -w-  Unicode Text format
    UBRL6 S  -w-  Unicode Text format 6dot
      UIL U  -w-  X-Motif UIL table
     UYVY S  rw-  16bit/pixel interleaved YUV
      VDA S  rw+  Truevision Targa image
    VICAR S  rw-  VICAR rasterfile format
      VID S  rw+  Visual Image Directory
     VIFF S  rw+  Khoros Visualization image
      VST S  rw+  Truevision Targa image
     WBMP S  rw-  Wireless Bitmap (level 0) image
      WPG U  r--  Word Perfect Graphics
      X3F S  r--  Foveon X3 (Sigma/Polaroid) RAW
      XBM S  rw-  X Windows system bitmap (black/white)
       XC P  r--  Constant image uniform color
      XCF S  r--  GIMP image
      XMP P  rw-  Adobe XML metadata
      XPM S  rw-  X Windows system pixmap (color)
       XV S  rw+  Khoros Visualization image
        Y S  rw+  Raw yellow samples
      YUV S  rw-  CCIR 601 4:1:1 or 4:2:2 (8-bit only)

 Meaning of 'L': P=Primary, S=Stable, U=Unstable

comment:12 follow-up: Changed 5 months ago by slabbe

The content of the second column is not the same in my case:

$ convert -list Format
   Format  Module    Mode  Description
-------------------------------------------------------------------------------
      3FR  DNG       r--   Hasselblad CFV/H3D39II
      3G2  MPEG      r--   Media Container
      3GP  MPEG      r--   Media Container
      AAI* AAI       rw+   AAI Dune image
[...]
   YCbCrA* YCbCr     rw+   Raw Y, Cb, Cr, and alpha samples
      YUV* YUV       rw-   CCIR 601 4:1:1 or 4:2:2

* native blob support
r read support
w write support
+ support for multiple images

Most of the doctests are doing a conversion (bunch of pngs) -> gif.

Do you think something based on

convert -list Format | grep PNG

could be a good test to detect that convert *.png output.gif works?

For info, for me it gives this:

      JNG* PNG       rw-   JPEG Network Graphics
      MNG* PNG       rw+   Multiple-image Network Graphics (libpng 1.6.37)
      PNG* PNG       rw-   Portable Network Graphics (libpng 1.6.37)
           See http://www.libpng.org/ for details about the PNG format.
    PNG00* PNG       rw-   PNG inheriting bit-depth, color-type from original if possible
    PNG24* PNG       rw-   opaque or binary transparent 24-bit RGB (zlib 1.2.11)
    PNG32* PNG       rw-   opaque or transparent 32-bit RGBA
    PNG48* PNG       rw-   opaque or binary transparent 48-bit RGB
    PNG64* PNG       rw-   opaque or transparent 64-bit RGBA
     PNG8* PNG       rw-   8-bit indexed with optional binary transparency

comment:13 in reply to: ↑ 12 Changed 5 months ago by mjo

Replying to slabbe:

Most of the doctests are doing a conversion (bunch of pngs) -> gif.

Do you think something based on

convert -list Format | grep PNG

I suppose, but you would need to somehow test for read support of PNGs and write support of GIFs. Personally I would just try to convert a 1x1 pixel from png to gif and see if it works.

comment:14 Changed 5 months ago by slabbe

Good idea, I will post a branch soon.

comment:15 Changed 5 months ago by slabbe

  • Authors set to Sébastien Labbé
  • Branch set to u/slabbe/33092
  • Commit set to e8dbdf9d5c5947eb54b9358247d20363f4e0427e

New commits:

ef3179033092: adding method is_functional to imagemagick feature
e8dbdf933092: print log when an error with convert command occurs in animate.py

comment:16 Changed 5 months ago by git

  • Commit changed from e8dbdf9d5c5947eb54b9358247d20363f4e0427e to 020e101ecbefd5b221f8417d438cf7a5da7b78ff

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

020e10133092: print log when an error with convert command occurs in animate.py

comment:17 Changed 5 months ago by slabbe

  • Status changed from new to needs_review

Needs review!

Also, I would be curious to see the following outputs with the current branch for your machine:

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_present()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().is_functional()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().require()

Also this allows to run tests on the 5 failings files and list the skipped doctects:

sage -tp --long --show-skipped src/sage/combinat/crystals/mv_polytopes.py src/sage/combinat/tiling.py src/sage/combinat/words/paths.py src/sage/plot/animate.py src/sage/plot/plot3d/tachyon.py

For comparison, for me, it gives this:

Doctesting 5 files using 8 threads.
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/plot/plot3d/tachyon.py
    1 test not run due to known bugs
    0 tests not run because we ran out of time
    [406 tests, 13.74 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/combinat/crystals/mv_polytopes.py
    0 tests not run because we ran out of time
    [67 tests, 24.64 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/combinat/words/paths.py
    2 internet tests not run
    1 test for not implemented functionality not run
    0 tests not run because we ran out of time
    [521 tests, 64.60 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/plot/animate.py
    4 not tested tests not run
    0 tests not run because we ran out of time
    [125 tests, 103.21 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/combinat/tiling.py
    5 not tested tests not run
    0 tests not run because we ran out of time
    [489 tests, 211.99 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 212.1 seconds
    cpu time: 271.1 seconds
    cumulative wall time: 418.2 seconds
Features detected for doctesting: ffmpeg,imagemagick

comment:18 Changed 5 months ago by slabbe

  • Summary changed from spkg-configure.m4 for ffmpeg/imagemagick need feature checks to ffmpeg/imagemagick features need feature checks

comment:19 Changed 5 months ago by slabbe

Most of the errors in the attached log were coming from imagemagick. I wait to get new inputs before touching ffmpeg features.

comment:20 follow-up: Changed 5 months ago by mjo

I get "not functional",

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_present()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().is_functional()
FeatureTestResult('imagemagick', False)
sage: ImageMagick().require()

but it looks like the # optional - ImageMagick tests are still being run. For example,

File "src/sage/plot/animate.py", line 168, in sage.plot.animate.Animation
Failed example:
    a[:5]             # optional -- ImageMagick
Expected:
    Animation with 5 frames
Got:
    Command 
       'sage-native-execute convert -dispose Background -delay 20 -loop 0 *.png "/home/mjo/.sage/temp/gantu/4697/tmp_tlrqhxiz.gif"'
    returned non-zero exit status 1.
    Here is the content of the stderr:convert convert: No decode delegate for this image format (00000000.png).
    Here is the content of the stdout:
    Animation with 5 frames

comment:21 in reply to: ↑ 20 Changed 5 months ago by slabbe

Replying to mjo:

I get "not functional",

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_present()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().is_functional()
FeatureTestResult('imagemagick', False)
sage: ImageMagick().require()

but it looks like the # optional - ImageMagick tests are still being run. For example,

I thought it was always the case that is_present() returns False when is_functional() returns False. But the above is a counterexample to this (it is a good thing, because it is more logical like that). I think my thinking was mislead by the way _is_present method is coded for the class Executable which is not consistent with the other type of features like the one here, see https://git.sagemath.org/sage.git/tree/src/sage/features/__init__.py?h=develop#n442

I think doctests are run for a feature when is_present() returns True which explains why the feature imagemagick are tested in your case. It should also check that it is functional as well.

Also, require() currently only check that is_present() returns True. This is the other reason I was thinking that is_present() should return False if is_functional() return False.

Last edited 5 months ago by slabbe (previous) (diff)

comment:22 Changed 5 months ago by git

  • Commit changed from 020e101ecbefd5b221f8417d438cf7a5da7b78ff to bd568fe0b2e7d2129ee50c5342505fc05b74f7ab

Branch pushed to git repo; I updated commit sha1. New commits:

bd568fe33092: move the is_functional method of class ImageMagick(JoinFeature) to a method of class convert(Excutable)

comment:23 follow-ups: Changed 5 months ago by slabbe

Okay, I found an alternate way of doing it. I moved the is_functional method into a class Executable for the convert as it is done within the file features/graphviz.py.

@mjo, can you test the same thing again?

Maybe changes needs to be done with respect to is_present(), is_functional() and require(). In particular, I have the following questions for @mkoeppe:

  • why Feature.require() does not check that is_functional() is True?
  • should is_present() always return False when is_functional return False (even if the vocabulary that is used should logically allow it)?

But this discussion should be done in another ticket.

comment:24 in reply to: ↑ 23 Changed 5 months ago by mkoeppe

Replying to slabbe:

Maybe changes needs to be done with respect to is_present(), is_functional() and require(). In particular, I have the following questions for @mkoeppe:

  • why Feature.require() does not check that is_functional() is True?
  • should is_present() always return False when is_functional return False (even if the vocabulary that is used should logically allow it)?

But this discussion should be done in another ticket.

I have opened #33114 for this discussion. I don't have special insights regarding the original design (done by @saraedum and revised by @jdemeyer) but I think it is likely that the design needs to be refined now that we are bringing it to more systematic use.

comment:25 in reply to: ↑ 23 ; follow-up: Changed 5 months ago by saraedum

  • Status changed from needs_review to needs_work

I don't think you are cleaning up the temporary files.

Have you considered using with TemporaryDirectory instead? See https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory.

This also lets you create a named file in the temporary directory and so you don't need to run with shell=True which might be considered a security issue by some.

comment:26 Changed 5 months ago by saraedum

  • Reviewers set to Julian Rüth

comment:27 follow-up: Changed 5 months ago by mjo

Better now:

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().require()
---------------------------------------------------------------------------
FeatureNotPresentError                    Traceback (most recent call last)
<ipython-input-2-501a65b4daea> in <module>
----> 1 ImageMagick().require()

~/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/features/__init__.py in require(self)
    204         presence = self.is_present()
    205         if not presence:
--> 206             raise FeatureNotPresentError(self, presence.reason, presence.resolution)
    207 
    208     def __repr__(self):

FeatureNotPresentError: imagemagick is not available.
Running convert on a sample file returned non-zero exit status 1
No equivalent system packages for gentoo are known to Sage.
To install imagemagick using the Sage package manager, you can try to run:
  !sage -i imagemagick
No equivalent system packages for pip are known to Sage.
Further installation instructions might be available at https://www.imagemagick.org/.

And the test failures are gone.

comment:28 Changed 5 months ago by git

  • Commit changed from bd568fe0b2e7d2129ee50c5342505fc05b74f7ab to d5e5ab024240026cb8d77a46a02ab16924e2ef51

Branch pushed to git repo; I updated commit sha1. New commits:

d5e5ab033092: improved the look of stderr and stdout when an error occurs

comment:29 in reply to: ↑ 27 ; follow-up: Changed 5 months ago by slabbe

Replying to mjo:

And the test failures are gone.

All of them? Even the one listed in the description of this ticket related to ffmpeg?

I added a small commit to improve the way errors are printed. I also added the stderr output to the output you obtain when ImageMagick().require() fails.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 5 months ago by mjo

Replying to slabbe:

Replying to mjo:

And the test failures are gone.

All of them? Even the one listed in the description of this ticket related to ffmpeg?

No, not the ffmpeg ones, obviously =)

I double-checked all of the # optional - ImageMagick tests and it looks like the only other format we use is ppm, but imagemagick support for ppm is apparently impossible to disable. So the existing test for a PNG conversion is probably all we need.

comment:31 in reply to: ↑ 30 Changed 5 months ago by slabbe

Replying to mjo:

No, not the ffmpeg ones, obviously =)

Ok! Please, can you attach the log of the remaining failures?

comment:32 in reply to: ↑ 25 ; follow-up: Changed 5 months ago by slabbe

Replying to saraedum:

I don't think you are cleaning up the temporary files.

Indeed, I am not. Should I?

Have you considered using with TemporaryDirectory instead? See https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory.

Well the files are created in the current temporary folder SAGE_TMP within DOT_SAGE folder using the methods implemented in the misc folder: from sage.misc.temporary_file import tmp_filename. This method is already using the tempfile module within some temporary directory with some SageMath salt. It is used all over the place in sage:

$ git grep tmp_filename | wc
    264    1325   20892

And I don't remember any of its usage to contain any line which deletes the temporary files created (do you know one such place?). The temporary folder and the temporary files for instance all pictures generated by sage during the session gets deleted when sage closes as shown in the example below:

sage: SAGE_TMP
l'/home/slabbe/.sage/temp/labbe-laptop/1043566'
sage: !ls /home/slabbe/.sage/temp/labbe-laptop/1043566
ecl
sage: plot(sin(x),0,10)
Launched png viewer for Graphics object consisting of 1 graphics primitive
sage: !ls /home/slabbe/.sage/temp/labbe-laptop/1043566
ecl  tmp_w8bb8pxh.png
sage:                                                                           
Exiting Sage (CPU time 0m1.39s, Wall time 0m44.98s).

$ ls /home/slabbe/.sage/temp/labbe-laptop/1043566
ls: impossible d'accéder à '/home/slabbe/.sage/temp/labbe-laptop/1043566': Aucun fichier ou dossier de ce type

Can you explain what is wrong with the current approach?

Last edited 5 months ago by slabbe (previous) (diff)

comment:33 follow-ups: Changed 5 months ago by mjo

I spoke too soon. I re-enabled PNG support in my imagemagick and now these fail:

File "src/sage/plot/animate.py", line 565, in sage.plot.animate.Animation.?
Failed example:
    with open(td + 'my_animation.gif', 'rb') as f: print(b'\x21\xf9\x04\x08\x23\x00' in f.read())  # optional -- ImageMagick
Expected:
    True
Got:
    False
File "src/sage/plot/animate.py", line 1105, in sage.plot.animate.Animation.save
Failed example:
    with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x14\x00' in f.read())  # optional -- ImageMagick
Expected:
    True
Got:
    False
File "src/sage/plot/animate.py", line 1112, in sage.plot.animate.Animation.save
Failed example:
    with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x23\x00' in f.read())  # optional -- ImageMagick
Expected:
    True
Got:
    False

I wonder what those tests are trying to check. Maybe instead we should just check for the magic "GIF8" bits at the beginning of the file...

comment:34 in reply to: ↑ 33 ; follow-up: Changed 5 months ago by slabbe

Replying to mjo:

I wonder what those tests are trying to check.

These tests are there since the very beginning of Sage. I think the idea is that it is difficult to test that a GIF is correct as we do not want a human to look at them each time. The idea is to only check that some binary string appear in the middle of the created gif file.

So if it is False, my first advice would be to actually look at the animation with your eyes, because possibly they are broken. Are they?

Maybe instead we should just check for the magic "GIF8" bits at the beginning of the file...

Depends on the answer to the first question.

comment:35 in reply to: ↑ 34 Changed 5 months ago by mjo

Replying to slabbe:

So if it is False, my first advice would be to actually look at the animation with your eyes, because possibly they are broken. Are they?

No, they look fine.

comment:36 in reply to: ↑ 33 Changed 5 months ago by mjo

Replying to mjo:

I wonder what those tests are trying to check. Maybe instead we should just check for the magic "GIF8" bits at the beginning of the file...

So long as we're already using imagemagick, something like

$ identify -format "%m" /home/mjo/.sage/temp/gantu/29047/dir_b77_l8ae/my_animation.gif
GIFGIFGIFGIFGIFGIFGIFGIFGIF

is nice and easy.

comment:37 in reply to: ↑ 32 ; follow-up: Changed 5 months ago by saraedum

Replying to slabbe:

Replying to saraedum:

I don't think you are cleaning up the temporary files.

Indeed, I am not. Should I?

Ok. Maybe that's the SageMath way of doing things. (I guess all this functionality is from Python <3.2 when there was no good support for temporary directories in Python.) If SageMath is cleaning up already, that's probably fine then. (For me $SAGE_TMP is annoying because it is often located on a remote drive...)

Can you explain what is wrong with the current approach?

I was originally just very confused about the *.png. I don't think you need this. You can just convert the file that you created directly instead, can't you? This will also not interact with other .png files that happen to exist in this directory.

I would really try to get rid of the shell=True, though. Apart from security concerns, shell=True makes things break in all kinds of weird ways in strangely configured environments.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 5 months ago by slabbe

Replying to saraedum:

Ok. Maybe that's the SageMath way of doing things. (I guess all this functionality is from Python <3.2 when there was no good support for temporary directories in Python.) If SageMath is cleaning up already, that's probably fine then. (For me $SAGE_TMP is annoying because it is often located on a remote drive...)

Well even older, the tmp_filename stuff dates back to the very early sagemath stuff on Python 2.x. After 15 years, it is possible that this can be done in a better way in 2022. But I would suggest to improve that in another ticket. Feel free to open one.

Can you explain what is wrong with the current approach?

I was originally just very confused about the *.png. I don't think you need this. You can just convert the file that you created directly instead, can't you? This will also not interact with other .png files that happen to exist in this directory.

The *.png was taken from the sage/plot/animate.py where a bunch of PNGs are turned into a animation. It is true here that I can replace the *.png by the only one file which is targeted here. Good idea. I will do this.

I would really try to get rid of the shell=True, though. Apart from security concerns, shell=True makes things break in all kinds of weird ways in strangely configured environments.

Ok, I will check if it works without. If it's there, it was for a reason I think. But let me check this tomorrow.

Last edited 5 months ago by slabbe (previous) (diff)

comment:39 in reply to: ↑ 38 Changed 5 months ago by saraedum

Ok, I will check if it works without. If it's there, it was for a reason I think. But let me check this tomorrow.

The shell=True is only there to make the *.png work I think.

Changed 5 months ago by mjo

comment:40 Changed 5 months ago by mjo

I attached another log file with just the remaining ffmpeg failures.

comment:41 Changed 5 months ago by git

  • Commit changed from d5e5ab024240026cb8d77a46a02ab16924e2ef51 to 3961423cf5e4c237e05039173f13a300fc1dfcf6

Branch pushed to git repo; I updated commit sha1. New commits:

7a2fdfd33092: now with shell=False
78fc81933092: adding is_functional method to feature ffmpeg
396142333092: improved the way errors are worded and returned

comment:42 Changed 5 months ago by git

  • Commit changed from 3961423cf5e4c237e05039173f13a300fc1dfcf6 to 9810e584f7b3673a1c7afaf4c55018380e114f51

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

df0a2d633092: adding is_functional method to feature ffmpeg
9810e5833092: improved the way errors are worded and returned

comment:43 Changed 5 months ago by slabbe

It is not ready for review. In the current state, the doctest framework is stock in an infinite loop with the command sage -t src/sage/features/ffmpeg.py, but it is fine with sage -t src/sage/features/imagemagick.py?

comment:44 Changed 5 months ago by slabbe

I still do not understand how the command sage -t src/sage/features/ffmpeg.py get stuck:

$ sage -t ffmpeg.py 
[...]
Doctesting 1 file.
sage -t --random-seed=168065381240033119635518975867815506494 ffmpeg.py

It comes from the optional tags because if I do:

  • src/sage/features/ffmpeg.py

    diff --git a/src/sage/features/ffmpeg.py b/src/sage/features/ffmpeg.py
    index a2661b33be..fdc638e1b9 100644
    a b class FFmpeg(Executable): 
    2121    EXAMPLES::
    2222
    2323        sage: from sage.features.ffmpeg import FFmpeg
    24         sage: FFmpeg().is_present()  # optional - ffmpeg
     24        sage: FFmpeg().is_present()
    2525        FeatureTestResult('ffmpeg', True)
    2626    """
    2727    def __init__(self):
    class FFmpeg(Executable): 
    4343        EXAMPLES::
    4444
    4545            sage: from sage.features.ffmpeg import FFmpeg
    46             sage: FFmpeg().is_functional()   # optional - ffmpeg
     46            sage: FFmpeg().is_functional()
    4747            FeatureTestResult('ffmpeg', True)
    4848
    4949        """

then, I get

$ sage -t ffmpeg.py 
[...]
Doctesting 1 file.
sage -t --random-seed=336902193762431828596947527190320053620 ffmpeg.py
    [6 tests, 0.11 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------

Can someone help understand the problem? The command sage -t ffmpeg.py sees there are optional tags ffmpeg which calls the method in the file ffmpeg.py and then what? This works for all other files in src/sage/features. Why doesn't it work for ffmpeg.py ?

comment:45 follow-up: Changed 5 months ago by slabbe

Regardless of the infinite loop bug while doctesting, the current state makes the following to work:

sage: from sage.features.ffmpeg import FFmpeg
sage: FFmpeg().is_present()
FeatureTestResult('ffmpeg', True)
sage: FFmpeg().is_functional()
FeatureTestResult('ffmpeg', True)
sage: FFmpeg().require()

@mjo: Can you provide the output of the above for your machine?

@mjo: Also, can you confirm you also have a infinite loop while testing sage -t src/sage/features/ffmpeg.py or maybe you won't have the infinite loop since the feature ffmpeg turns out not to be present and functional. If no infinite loop, you can also report whether sage -t src/sage/plot/animate.py looks better.

Last edited 5 months ago by slabbe (previous) (diff)

comment:46 in reply to: ↑ 45 ; follow-up: Changed 5 months ago by mjo

Replying to slabbe:

@mjo: Can you provide the output of the above for your machine?

Looks like it should with the busted ffmpeg:

sage: from sage.features.ffmpeg import FFmpeg
sage: FFmpeg().is_present()
FeatureTestResult('ffmpeg', False)
sage: FFmpeg().is_functional()
FeatureTestResult('ffmpeg', False)
sage: FFmpeg().require()
---------------------------------------------------------------------------
FeatureNotPresentError                    Traceback (most recent call last)
...

@mjo: Also, can you confirm you also have a infinite loop while testing sage -t src/sage/features/ffmpeg.py

But I also get the infinite loop. Should be a fun mystery since I don't see anything obviously wrong in ffmpeg.py.

comment:47 in reply to: ↑ 46 Changed 5 months ago by slabbe

Replying to mjo:

FeatureNotPresentError                    Traceback (most recent call last)
...

Actually, could you paste the full output of FFmpeg().require()? I have been trying to improve the way the error is printed. In ffmpeg.log, an error is reported, but we can't see what exactly. This should be better now.

But I also get the infinite loop. Should be a fun mystery since I don't see anything obviously wrong in ffmpeg.py.

Do you have an infinite loop when you run sage -t src/sage/plot/animate.py ?

comment:48 Changed 5 months ago by mjo

Ok:

sage: from sage.features.ffmpeg import FFmpeg
sage: FFmpeg().require()
---------------------------------------------------------------------------
FeatureNotPresentError                    Traceback (most recent call last)
<ipython-input-2-e403f9ac9adf> in <module>
----> 1 FFmpeg().require()

~/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/features/__init__.py in require(self)
    204         presence = self.is_present()
    205         if not presence:
--> 206             raise FeatureNotPresentError(self, presence.reason, presence.resolution)
    207 
    208     def __repr__(self):

FeatureNotPresentError: ffmpeg is not available.
Running command "['ffmpeg', '-y', '-f', 'image2', '-r', '5', '-i', 'tmp_w2ac36zy.png', '-pix_fmt', 'rgb24', '-loop', '0', 'tmp_w2ac36zy.gif']" returned non-zero exit status "1" with stderr "ffmpeg version 4.4.1 Copyright (c) 2000-2021 the FFmpeg developers
  built with gcc 11.2.0 (Gentoo Hardened 11.2.0 p1)
  configuration: --prefix=/usr --libdir=/usr/lib64 --shlibdir=/usr/lib64 --docdir=/usr/share/doc/ffmpeg-4.4.1-r1/html --mandir=/usr/share/man --enable-shared --cc=x86_64-pc-linux-gnu-gcc --cxx=x86_64-pc-linux-gnu-g++ --ar=x86_64-pc-linux-gnu-ar --nm=x86_64-pc-linux-gnu-nm --ranlib=x86_64-pc-linux-gnu-ranlib --pkg-config=x86_64-pc-linux-gnu-pkg-config --optflags='-O2 -pipe -march=native -fuse-ld=mold' --extra-libs= --disable-static --enable-avfilter --enable-avresample --disable-stripping --disable-optimizations --disable-libcelt --disable-encoders --disable-indev=v4l2 --disable-outdev=v4l2 --disable-indev=alsa --disable-indev=oss --disable-indev=jack --disable-indev=sndio --disable-outdev=alsa --disable-outdev=oss --disable-outdev=sndio --disable-bzlib --disable-runtime-cpudetect --disable-debug --disable-gcrypt --disable-gnutls --disable-gmp --disable-gpl --disable-hardcoded-tables --disable-iconv --disable-libxml2 --disable-lzma --disable-network --disable-opencl --disable-openssl --disable-postproc --disable-libsmbclient --disable-ffplay --disable-sdl2 --disable-vaapi --disable-vdpau --disable-vulkan --disable-xlib --disable-libxcb --disable-libxcb-shm --disable-libxcb-xfixes --disable-zlib --disable-libcdio --disable-libiec61883 --disable-libdc1394 --disable-libcaca --disable-openal --disable-opengl --disable-libv4l2 --disable-libpulse --disable-libdrm --disable-libjack --disable-libopencore-amrwb --disable-libopencore-amrnb --disable-libcodec2 --disable-libdav1d --disable-libfdk-aac --disable-libopenjpeg --disable-libbluray --disable-libgme --disable-libgsm --disable-libaribb24 --disable-mmal --disable-libmodplug --disable-libopus --disable-libilbc --disable-librtmp --disable-libssh --disable-libspeex --disable-libsrt --disable-librsvg --disable-ffnvcodec --disable-libvorbis --disable-libvpx --disable-libzvbi --disable-appkit --disable-libbs2b --disable-chromaprint --disable-cuda-llvm --disable-libflite --disable-frei0r --disable-libfribidi --disable-fontconfig --disable-ladspa --disable-libass --disable-libtesseract --disable-lv2 --disable-libfreetype --disable-libvidstab --disable-librubberband --disable-libzmq --disable-libzimg --disable-libsoxr --disable-pthreads --disable-armv5te --disable-armv6 --disable-armv6t2 --disable-neon --disable-vfp --disable-vfpv3 --disable-armv8 --disable-mipsdsp --disable-mipsdspr2 --disable-mipsfpu --disable-altivec --disable-vsx --disable-power8 --disable-amd3dnow --disable-amd3dnowext --disable-aesni --disable-avx --disable-avx2 --disable-fma3 --disable-fma4 --disable-mmx --disable-mmxext --disable-sse --disable-sse2 --disable-sse3 --disable-ssse3 --disable-sse4 --disable-sse42 --disable-xop --cpu=host --disable-doc --disable-htmlpages --enable-manpages
  libavutil      56. 70.100 / 56. 70.100
  libavcodec     58.134.100 / 58.134.100
  libavformat    58. 76.100 / 58. 76.100
  libavdevice    58. 13.100 / 58. 13.100
  libavfilter     7.110.100 /  7.110.100
  libavresample   4.  0.  0 /  4.  0.  0
  libswscale      5.  9.100 /  5.  9.100
  libswresample   3.  9.100 /  3.  9.100
[image2 @ 0x55f828dd4680] Could not find codec parameters for stream 0 (Video: png, none): unspecified size
Consider increasing the value for the 'analyzeduration' (0) and 'probesize' (5000000) options
Input #0, image2, from 'tmp_w2ac36zy.png':
  Duration: 00:00:00.20, start: 0.000000, bitrate: N/A
  Stream #0:0: Video: png, none, 5 fps, 5 tbr, 5 tbn, 5 tbc
Automatic encoder selection failed for output stream #0:0. Default encoder for format gif (codec gif) is probably disabled. Please choose an encoder manually.
Error selecting an encoder for stream 0:0" and stdout "".
No equivalent system packages for gentoo are known to Sage.
To install ffmpeg using the Sage package manager, you can try to run:
  !sage -i ffmpeg
No equivalent system packages for pip are known to Sage.
Further installation instructions might be available at https://www.ffmpeg.org/.

comment:49 follow-up: Changed 5 months ago by mjo

You can eliminate the hang with,

  • src/sage/features/ffmpeg.py

    diff --git a/src/sage/features/ffmpeg.py b/src/sage/features/ffmpeg.py
    index a2661b33be..aa818a9e79 100644
    a b class FFmpeg(Executable): 
    7575
    7676        # running the command (taken from sage/plot/animate.py)
    7777        from subprocess import run
    78         cmd = ['ffmpeg', '-y', '-f', 'image2', '-r', '5', '-i',
    79                filename_png, '-pix_fmt', 'rgb24', '-loop', '0',
    80                filename_gif]
     78        cmd = ['convert', '-dispose', 'Background', '-delay', '20',
     79                '-loop', '0', filename_png, filename_gif]
    8180        result = run(cmd, cwd=base, capture_output=True, text=True)
    8281
    8382        # If an error occured, return False

which is pretty amazing to me.

comment:50 follow-up: Changed 5 months ago by slabbe

Ok, I think I have found the loop which loops over and over. With the following change

  • src/sage/doctest/forker.py

    diff --git a/src/sage/doctest/forker.py b/src/sage/doctest/forker.py
    index 6f49a48021..b9c6544584 100644
    a b class DocTestDispatcher(SageObject): 
    17871787                    sel.__exit__(None, None, None)
    17881788
    17891789                while True:
     1790                    print('infinite loop is here')
    17901791                    # To avoid calling time.time() all the time while
    17911792                    # checking for timeouts, we call it here, once per
    17921793                    # loop. It's not a problem if this isn't very

then, I get:

$ sage -t src/sage/features/ffmpeg.py 
[...]
Doctesting 1 file.
infinite loop is here
infinite loop is here
sage -t --random-seed=86298206900732853338809268896610660473 ../features/ffmpeg.py
infinite loop is here
infinite loop is here
infinite loop is here
infinite loop is here
infinite loop is here
infinite loop is here
infinite loop is here
infinite loop is here
infinite loop is here
...

Also, the log of forker.py indicates that a recent change was made on it at #33032. I don't know if it is related.

comment:51 in reply to: ↑ 50 Changed 5 months ago by slabbe

Replying to slabbe:

Also, the log of forker.py indicates that a recent change was made on it at #33032. I don't know if it is related.

Undoing #33032 does not change anything. It is unrelated.

comment:52 in reply to: ↑ 49 Changed 5 months ago by slabbe

Replying to mjo:

You can eliminate the hang with, [...] which is pretty amazing to me.

Conversely, the following creates an hang for sage -t src/sage/features/imagemagick.py:

  • src/sage/features/imagemagick.py

    diff --git a/src/sage/features/imagemagick.py b/src/sage/features/imagemagick.py
    index 0b58946919..5de9ef679d 100644
    a b class convert(Executable): 
    7979
    8080        # running command convert (taken from sage/plot/animate.py)
    8181        from subprocess import run
    82         cmd = ['convert', '-dispose', 'Background', '-delay', '20',
    83                 '-loop', '0', filename_png, filename_gif]
     82        cmd = ['ffmpeg', '-y', '-f', 'image2', '-r', '5', '-i',
     83               filename_png, '-pix_fmt', 'rgb24', '-loop', '0',
     84               filename_gif]
    8485        result = run(cmd, cwd=base, capture_output=True, text=True)
    8586
    8687        # If an error occured, return False

What does it mean?

Last edited 5 months ago by slabbe (previous) (diff)

comment:53 Changed 5 months ago by slabbe

Something seems to be wrong in the parallel_dispatch method of the doctest/forker.py file because using serial_dispatch works fine:

$ sage -t --serial src/sage/features/ffmpeg.py 
[...]
Doctesting 1 file.
sage -t --random-seed=282697849359684697319918281520742422437 src/sage/features/ffmpeg.py
    [6 tests, 0.06 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Features detected for doctesting: ffmpeg

Note that the --serial option is not mentionned in the doc of sage -t -h, but it is mentionned in the dispatch method in the forker.py file.

I still don't know how the ffmpeg.py file produces the infinite loop for sage -t.

comment:54 Changed 5 months ago by slabbe

Doing small progres... It seems the chosen command to test ffmpeg hangs when called within the doctest framework in a non serial way (which is the default):

i.e., with

  • src/sage/features/ffmpeg.py

    diff --git a/src/sage/features/ffmpeg.py b/src/sage/features/ffmpeg.py
    index a2661b33be..105572849d 100644
    a b class FFmpeg(Executable): 
    7878        cmd = ['ffmpeg', '-y', '-f', 'image2', '-r', '5', '-i',
    7979               filename_png, '-pix_fmt', 'rgb24', '-loop', '0',
    8080               filename_gif]
     81        print('before')
    8182        result = run(cmd, cwd=base, capture_output=True, text=True)
     83        print('after')
    8284
    8385        # If an error occured, return False
    8486        if result.returncode:

it gives:

$ sage -t --serial src/sage/features/ffmpeg.py 
...
Doctesting 1 file.
sage -t --random-seed=307360575550711922927379583574768772711 src/sage/features/ffmpeg.py
before
after
**********************************************************************
File "src/sage/features/ffmpeg.py", line 46, in sage.features.ffmpeg.FFmpeg.is_functional
Failed example:
    FFmpeg().is_functional()   # optional - ffmpeg
Expected:
    FeatureTestResult('ffmpeg', True)
Got:
    before
    after
    FeatureTestResult('ffmpeg', True)
**********************************************************************
1 item had failures:
   1 of   3 in sage.features.ffmpeg.FFmpeg.is_functional
    [6 tests, 1 failure, 0.06 s]

but

$ sage -t src/sage/features/ffmpeg.py 
...
Doctesting 1 file.
before
sage -t --random-seed=9443241230115374037438588873102610711 src/sage/features/ffmpeg.py
^CKilling test src/sage/features/ffmpeg.py
----------------------------------------------------------------------
Doctests interrupted: 0/1 files tested
----------------------------------------------------------------------

only prints before.

comment:55 Changed 5 months ago by slabbe

Replacing the ffmpeg test command by something simpler like cmd = ['ffmpeg', '-i', filename_png, filename_gif] still hangs.

comment:56 Changed 5 months ago by slabbe

The documentation of subprocess.run says that it "run the command described by args. Wait for command to complete, then return a CompletedProcess instance."

Maybe the fact of running the doctests within parallel threads makes it so that the ffmpeg command run by run does not complete?

comment:58 Changed 5 months ago by git

  • Commit changed from 9810e584f7b3673a1c7afaf4c55018380e114f51 to 34c2bb3268ea7129a4d80a9d711384b7a47bebfb

Branch pushed to git repo; I updated commit sha1. New commits:

34c2bb333092: ffmpeg-hangs-when-run-in-background

comment:59 follow-up: Changed 5 months ago by slabbe

  • Status changed from needs_work to needs_review

@mjo: Are the only remainings issues the one reported in https://trac.sagemath.org/ticket/33092#comment:33 ?

comment:60 in reply to: ↑ 59 Changed 5 months ago by mjo

Replying to slabbe:

Got it! https://stackoverflow.com/questions/16523746/ffmpeg-hangs-when-run-in-background

Congrats =)

I spent a long time on that today before I got dragged away on work.

@mjo: Are the only remainings issues the one reported in https://trac.sagemath.org/ticket/33092#comment:33 ?

There are still some ffmpeg issues. Unlike with imagemagick, we try several different formats with ffmpeg and all need to be supported. For example, having re-enabled whatever I needed to get is_functional() to pass,

File "src/sage/plot/animate.py", line 806, in sage.plot.animate.Animation.show
Failed example:
    a.show(format="webm")        # optional -- ffmpeg
Exception raised:
    Traceback (most recent call last):
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.animate.Animation.show[2]>", line 1, in <module>
        a.show(format="webm")        # optional -- ffmpeg
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/plot/animate.py", line 845, in show
        dm.display_immediately(self, **kwds)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/repl/rich_output/display_manager.py", line 851, in display_immediately
        plain_text, rich_output = self._rich_output_formatter(obj, rich_repr_kwds)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/repl/rich_output/display_manager.py", line 643, in _rich_output_formatter
        rich_output = self._call_rich_repr(obj, rich_repr_kwds)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/repl/rich_output/display_manager.py", line 601, in _call_rich_repr
        return obj._rich_repr_(self, **rich_repr_kwds)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/plot/animate.py", line 747, in _rich_repr_
        self.save(filename, **kwds)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/plot/animate.py", line 1139, in save
        self.ffmpeg(savefile=filename, show_path=show_path, **kwds)
      File "/home/mjo/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/plot/animate.py", line 984, in ffmpeg
        check_call(cmd, shell=True, stderr=set_stderr)
      File "/usr/lib/python3.9/subprocess.py", line 373, in check_call
        raise CalledProcessError(retcode, cmd)
    subprocess.CalledProcessError: Command 'cd "/home/mjo/.sage/temp/gantu/18412/dir_674m51c2/"; sage-native-execute ffmpeg -y -f image2  -i /home/mjo/.sage/temp/gantu/18412/dir_674m51c2/%08d.png  /home/mjo/.sage/temp/gantu/18412/tmp_9iod6cgx.webm' returned non-zero exit status 1.

I would imagine we need similar checks for

sage: a.show(format="ogg")         # optional -- ffmpeg
sage: a.show(format="mp4")         # optional -- ffmpeg

and I'll try to confirm that.

comment:61 Changed 5 months ago by git

  • Commit changed from 34c2bb3268ea7129a4d80a9d711384b7a47bebfb to 9287a872f298a250efb82b8c618254e6ce5d9af0

Branch pushed to git repo; I updated commit sha1. New commits:

9287a8733092: testing all possible outputs for ffmpeg in is_functional

comment:62 Changed 5 months ago by slabbe

I added all outputs that I can see in the ffmpeg.log in which we can see two different kind of commands invoking ffmpeg. Only the format '.mpg' is incompatible with rbg24 parameter.

On my machine, require() now takes 1s, but it is cached:

sage: from sage.features.ffmpeg import FFmpeg
sage: %time FFmpeg().require()
CPU times: user 15.5 ms, sys: 69.2 ms, total: 84.7 ms
Wall time: 1.02 s
sage: %time FFmpeg().require()
CPU times: user 16 µs, sys: 0 ns, total: 16 µs
Wall time: 17.6 µs

comment:63 Changed 5 months ago by mjo

The latest commit is working for me (at least as far as ffmpeg is concerned). The only other test that I was able to make fail was the ogg one. The others like flash and wmv seem to pass anyway even though I definitely don't have support for those formats.

comment:64 follow-up: Changed 5 months ago by mjo

I would rather have this once during ./configure than in each invocation of sage -t, but that's a problem for another day:

sage: from sage.features.ffmpeg import FFmpeg
sage: %time FFmpeg().require()
CPU times: user 89.6 ms, sys: 288 ms, total: 378 ms
Wall time: 3 s

In other news, I finally figured out how to get ffmpeg to write a webm file.

comment:65 Changed 5 months ago by mjo

One more thing. If I build both a working ffmpeg AND a working imagemagick... the whole thing times out because the file takes ~440s and the default timeout is 300s:

$ sage -t src/sage/plot/animate.py 
Running doctests with ID 2022-01-05-17-59-38-b1f4a523.
Git branch: u/slabbe/33092
Using --optional=build,dochtml,gentoo,pip,sage,sage_spkg
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,ffmpeg,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,pandoc,pdf2svg,plantri,pynormaliz,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.plot,sage.rings.number_field,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,tdlib
Doctesting 1 file.
sage -t --warn-long 318.1 --random-seed=248859399066678770036022680812692330020 src/sage/plot/animate.py
    Timed out
...
Total time for all tests: 300.8 seconds

#32973 is a quick band-aid.

comment:66 Changed 5 months ago by slabbe

ffmpeg tests that times out are tracked at #33045. Should these be dealt here or there?

comment:67 in reply to: ↑ 64 ; follow-up: Changed 5 months ago by slabbe

Replying to mjo:

I would rather have this once during ./configure than in each invocation of sage -t, but that's a problem for another day:

sage: from sage.features.ffmpeg import FFmpeg
sage: %time FFmpeg().require()
CPU times: user 89.6 ms, sys: 288 ms, total: 378 ms
Wall time: 3 s

Maybe the current is_functional() for ffmpeg is exagerated. Maybe we can simplify and test only few well-chosen formats? If so, which formats do you think?

Or make them choose some at random? But, I guess this is not good as we may want is_functional to be consistent and reproducible.

comment:68 in reply to: ↑ 67 Changed 5 months ago by mjo

Replying to slabbe:

Maybe the current is_functional() for ffmpeg is exagerated. Maybe we can simplify and test only few well-chosen formats? If so, which formats do you think?

It's OK, we have to test everything we use in a doctest. It's possible that on a machine with e.g. wmv support, show(format="wmv") will actually try to use it, so we should leave those feature check in. At least until someone takes the time to evaluate if those doctests/examples are actually useful.

comment:69 follow-up: Changed 5 months ago by gh-sheerluck

ffmpeg without theora fails with Default encoder for format ogg (codec theora) is probably disabled

ffmpeg with theora passes all tests

sage -t --random-seed=35 src/sage/combinat/crystals/mv_polytopes.py
    [67 tests, 35.32 s]
sage -t --random-seed=35 src/sage/combinat/tiling.py
    [466 tests, 523.42 s]
sage -t --random-seed=35 src/sage/combinat/words/paths.py
    [513 tests, 130.42 s]
sage -t --random-seed=35 src/sage/doctest/external.py
    [41 tests, 2.32 s]
sage -t --random-seed=35 src/sage/features/ffmpeg.py
    [6 tests, 2.93 s]
sage -t --random-seed=35 src/sage/features/imagemagick.py
    [11 tests, 0.12 s]
sage -t --random-seed=35 src/sage/features/latex.py
    [19 tests, 0.06 s]
sage -t --random-seed=35 src/sage/plot/animate.py
    [232 tests, 183.16 s]
sage -t --random-seed=35 src/sage/plot/plot3d/tachyon.py
    [404 tests, 15.03 s]

comment:70 in reply to: ↑ 69 ; follow-up: Changed 5 months ago by slabbe

Replying to gh-sheerluck:

ffmpeg without theora fails with Default encoder for format ogg (codec theora) is probably disabled

Ok, this shows good things. For example, with the changes made, it does not swallow the error message anymore, so now we know why.

Also, something looks weird to me. Let me copy-paste the following part to explain:

sage -t --random-seed=262088897290595662578101588392178009818 src/sage/features/ffmpeg.py
**********************************************************************
File "src/sage/features/ffmpeg.py", line 24, in sage.features.ffmpeg.FFmpeg
Failed example:
    FFmpeg().is_present()
Expected:
    FeatureTestResult('ffmpeg', True)
Got:
    FeatureTestResult('ffmpeg', False)
**********************************************************************
File "src/sage/features/ffmpeg.py", line 46, in sage.features.ffmpeg.FFmpeg.is_functional
Failed example:
    FFmpeg().is_functional()
Expected:
    FeatureTestResult('ffmpeg', True)
Got:
    FeatureTestResult('ffmpeg', False)
**********************************************************************
2 items had failures:
   1 of   3 in sage.features.ffmpeg.FFmpeg
   1 of   3 in sage.features.ffmpeg.FFmpeg.is_functional
    [6 tests, 2 failures, 0.76 s]

So FFmpeg().require() works well in the sense that it detects that something does not work well with ffmpeg (related to ogg format). But then, this status should imply that doctests with optional tag ffmpeg are not run. Related to that, I observe above that the optional tag ffmpeg has disappeared:

    FFmpeg().is_present()

as opposed to

    FFmpeg().is_present() # optional -- ffmpeg

This may explain that those tests are run while they should not. So why are the optional ffmpeg tags gone?

comment:71 in reply to: ↑ 70 ; follow-up: Changed 5 months ago by gh-sheerluck

Replying to slabbe:

So why are the optional ffmpeg tags gone?

I deleted them I wanted to run all tests.

comment:72 in reply to: ↑ 71 Changed 5 months ago by slabbe

Replying to gh-sheerluck:

Replying to slabbe:

So why are the optional ffmpeg tags gone?

I deleted them I wanted to run all tests.

Note that you can use --optional=sage,ffmpeg if you want to test all doctests with no tags + doctests with tag ffmpeg. Also you can use --optional=sage,optional,ffmpeg to also test all optional features available on the machine while still forcing the test of doctests tagged with ffmpeg. I think this allows you to do the same thing without editing/removing the tags in the source code.

Last edited 5 months ago by slabbe (previous) (diff)

comment:73 follow-ups: Changed 5 months ago by gh-sheerluck

Thank you, with very long

--optional=arch,mathics,argcomplete,build,dochtml,pip,sage,sage_spkg,polymake,ffmpeg,imagemagick,latex,pandoc

for src/sage/combinat/tiling.py I got 489 tests passed, even more than 466 tests in comment 69.

I'm positive this ticket is ready for positive review :)

comment:74 in reply to: ↑ 73 ; follow-up: Changed 5 months ago by mjo

Replying to gh-sheerluck:

I'm positive this ticket is ready for positive review :)

I still have the failures in comment:33 with my "normal" imagemagick. I would try b"GIF8" in f.read() rather than looking for a specific binary sequence.

comment:75 in reply to: ↑ 73 Changed 5 months ago by slabbe

Replying to gh-sheerluck:

Thank you, with very long

--optional=arch,mathics,argcomplete,build,dochtml,pip,sage,sage_spkg,polymake,ffmpeg,imagemagick,latex,pandoc

for src/sage/combinat/tiling.py I got 489 tests passed, even more than 466 tests in comment 69.

I also get 489 tests passed (with --long) for that file. Note that you can use --show-skipped to list how many tests of each tag were skipped.

comment:76 Changed 5 months ago by git

  • Commit changed from 9287a872f298a250efb82b8c618254e6ce5d9af0 to 7cb0ea3e1065fcf6a47d6cc1329ca2bc0701eb16

Branch pushed to git repo; I updated commit sha1. New commits:

7cb0ea333092: in few doctests, only check that GIF8 appears in a generated gif file

comment:77 in reply to: ↑ 74 Changed 5 months ago by slabbe

Replying to mjo:

I still have the failures in comment:33 with my "normal" imagemagick. I would try b"GIF8" in f.read() rather than looking for a specific binary sequence.

Thanks for suggesting an alternative. I did just that in a commit. I hope I fixed the good lines.

comment:78 Changed 5 months ago by mjo

All passing now, thank you!

I have one last nit: I think class convert(Executable) should be capitalized for PEP8. And this new code is commented-out, but I don't mind if you want to keep it:

+        # Alternate way of raising the error (less verbose)
+        #result.check_returncode()

comment:79 Changed 5 months ago by git

  • Commit changed from 7cb0ea3e1065fcf6a47d6cc1329ca2bc0701eb16 to 1678c7f2f1c6ae3d6c870ed653e55c31f23e2476

Branch pushed to git repo; I updated commit sha1. New commits:

e15197f33092: capitalize class name to respect PEP8
1678c7f33092: removed unnecessary comment

comment:80 Changed 5 months ago by slabbe

Thanks for catching these. I did 2 commits.

comment:81 Changed 5 months ago by mjo

  • Reviewers changed from Julian Rüth to Julian Rüth, Michael Orlitzky
  • Status changed from needs_review to positive_review

Thanks again.

comment:82 Changed 4 months ago by slabbe

  • Priority changed from major to critical

I think this ticket should go to 9.5, but it seems for the 9.5 cycle, there was no period within the early rc candidates which was devoted to merging a bunch of tickets fixing just "defects". I think the 9.5 version would be better if more tickets of type "defect" with positive review were merged.

comment:83 Changed 4 months ago by mjo

  • Priority changed from critical to blocker

Yes, otherwise the 9.5 test suite is going to fail all over the place. Same for the lrslib ticket.

comment:84 Changed 4 months ago by mjo

(and it's a regression, since ffmpeg tests weren't enabled in 9.4 when ffmpeg was disabled)

comment:85 Changed 4 months ago by vbraun

  • Branch changed from u/slabbe/33092 to 1678c7f2f1c6ae3d6c870ed653e55c31f23e2476
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:86 Changed 4 months ago by vbraun

  • Commit 1678c7f2f1c6ae3d6c870ed653e55c31f23e2476 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

Probably an improvement but still broken on the buildbot: followup at #33219

Last edited 4 months ago by vbraun (previous) (diff)

comment:87 Changed 4 months ago by vbraun

  • Resolution set to fixed
  • Status changed from new to closed

comment:88 Changed 4 months ago by slabbe

Another follow-up to fix an issue observed by Justin on MacOSX needs review at #33231

Note: See TracTickets for help on using tickets.