Page MenuHomePhabricator

Add STL engine
ClosedPublic

Authored by MarkTraceur on Jul 28 2017, 3:46 PM.

Details

Reviewers
Gilles
fgiunchedi
Commits
rTHMBREXT69a3d6d63c9a: Add STL engine
Patch without arc
git checkout -b D732 && curl -L https://phabricator.wikimedia.org/D732?download=true | git apply
Summary

This adds an engine for STL files, which runs them through 3d2png. Detection of STL files is accomplished by checking the entire file size as compared to the triangle count in the header. In the case of a Swift loader, however, we short-circuit that detection by simply adding 'solid' to the beginning of the comment section of the header.

Diff Detail

Repository
rTHMBREXT Thumbor Plugins
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

MarkTraceur created this revision.Jul 28 2017, 3:46 PM
Restricted Application added a reviewer: Gilles. · View Herald TranscriptJul 28 2017, 3:46 PM
MarkTraceur updated this revision to Diff 1924.Jul 28 2017, 3:59 PM

Fixed tests.

fgiunchedi added inline comments.
wikimedia_thumbor/engine/stl/stl.py
13

copy/pasta

67

We'll have to test this extensively btw with xvfb (re)spawing, in case xvfb is implicated in the current pdfrenderer problems we've come across in https://phabricator.wikimedia.org/T159922
Also testing that xvfb's displays don't conflict when launched by two thumbor instances in parallel

fgiunchedi added inline comments.
wikimedia_thumbor/engine/stl/stl.py
67

I checked pdfrender and it uses xpra because xvfb requires a suid root binary btw, probably we want to do the same here. Also we'll likely need xvfb/xpra running separatedly, invoking the xserver for each request has likely too much overhead (a hunch, to be tested)

Gilles added inline comments.Aug 7 2017, 2:42 PM
wikimedia_thumbor/engine/stl/stl.py
67

Which binary is suid? xvfb-run doesn't seem to need that.

gilles@deployment-imagescaler01:~$ ls -al /usr/bin/xvfb-run
-rwxr-xr-x 1 root root 5711 Jul  6 20:49 /usr/bin/xvfb-run

The overhead of spawning the server seems very small:

gilles@deployment-imagescaler01:~$ time DISPLAY=:99 /usr/bin/xvfb-run -a -s "-ac screen 0 1280x1024X24" echo "foo"
foo

real	0m0.084s
user	0m0.008s
sys	0m0.024s
MarkTraceur updated this revision to Diff 1949.Aug 8 2017, 2:36 PM

Added triangles check, and gave each process its own display.

MarkTraceur updated this revision to Diff 1950.Aug 8 2017, 2:47 PM

Added binary STL hack for swift loader

MarkTraceur retitled this revision from [WIP] Add STL engine to Add STL engine.Aug 8 2017, 2:49 PM
MarkTraceur edited the summary of this revision. (Show Details)
Gilles edited edge metadata.Aug 8 2017, 2:58 PM

Looks right to me at first glance, you can test the hack by adding the same one to our https loader, and adding a test to test_https_loader.py pointing to an STL file hosted on one of our wikis.

wikimedia_thumbor/engine/stl/stl.py
79

I've read stuff about this again and I believe using the -n option for xvfb-run with the pid should be enough (no -a), and you would need to set the env variable anymore. Line 76 isn't the way you normally pass env variables in Python anyway.

MarkTraceur marked 5 inline comments as done.Aug 8 2017, 3:06 PM
MarkTraceur updated this revision to Diff 1951.Aug 8 2017, 3:06 PM

Changed the invocation for xvfb-run

Gilles added inline comments.Aug 8 2017, 4:35 PM
tests/integration/__init__.py
50

Should be 3D2PNG_PATH for consistency

wikimedia_thumbor/engine/stl/stl.py
74

That function is outdated, you must have gotten that from old code:

  File "/vagrant/tmp/thumbor-plugins/wikimedia_thumbor/engine/stl/stl.py", line 73, in create_image
    self.prepare_temp_files(buffer)
AttributeError: 'Engine' object has no attribute 'prepare_temp_files'

Lines 72 and 73 should now be replaced with:

self.prepare_source(buffer)
78

Just checked and the -a option is needed in addition to -n after all...

79

You need to convert the pid to a string, popen only accepts string arguments

82

The parameters for 3d2png are wrong:

Usage: 3drender <model> <dimensions> <output.png>

Additionally, you'll need to create a temp file path for the output png, and read it into a buffer to pass it to create_image below.

83

request.height isn't set most of the time, which results in x0 here. I believe that when using 3d2png in Mediawiki we set an arbitrary aspect ratio anyway. So just use that ratio and calculate the height based on self.context.request.width.

84

Same for this, it's outdated and should just be self.source now.

87

Another outdated call, needs to be self.command

Gilles added a comment.Aug 8 2017, 4:46 PM

You should be able to test the code on deployment-imagescaler01:

gilles@deployment-imagescaler01:~$ git clone https://phabricator.wikimedia.org/diffusion/THMBREXT/thumbor-plugins.git
gilles@deployment-imagescaler01:~$ cd thumbor-plugins/
gilles@deployment-imagescaler01:~/thumbor-plugins$ git checkout -b D732 && curl -L https://phabricator.wikimedia.org/D732?download=true | git apply
gilles@deployment-imagescaler01:~/thumbor-plugins$ PATH=$PATH:/srv/deployment/3d2png/deploy/src nosetests tests/integration/test_types.py:WikimediaTest.test_stl_text
tests/integration/test_types.py
261

There's a case mismatch here, the original has a lowercase c.

MarkTraceur marked 8 inline comments as done.Aug 8 2017, 5:18 PM
MarkTraceur added inline comments.
tests/integration/__init__.py
50

It wouldn't work, variables can't start with numbers. Compromised on THREED2PNG_PATH

MarkTraceur updated this revision to Diff 1952.Aug 8 2017, 5:19 PM
MarkTraceur marked an inline comment as done.

Fix old methods, 3d2png syntax

Gilles added inline comments.Aug 8 2017, 5:23 PM
wikimedia_thumbor/engine/stl/stl.py
101

You should use ShellRunner.rm_f which is more failsafe, as well as run it if self.command throws a CommandError.

MarkTraceur updated this revision to Diff 1953.Aug 8 2017, 5:24 PM
MarkTraceur marked an inline comment as done.

Change test file to correct case

MarkTraceur marked an inline comment as done.Aug 8 2017, 5:25 PM
MarkTraceur marked an inline comment as done.Aug 8 2017, 5:31 PM
MarkTraceur updated this revision to Diff 1954.Aug 8 2017, 5:33 PM

Fix temp file removal and add crystal thumbnail

MarkTraceur updated this revision to Diff 1955.Aug 8 2017, 5:34 PM

Oops, forgot one case change.

Gilles added a comment.Aug 8 2017, 5:46 PM

This code in 3d2png is getting in the way: https://github.com/wikimedia/3d2png/blob/master/3d2png.js#L68 because the temp file generated by the thumbor plugins is extension-less.

My advice would be to get rid of that defensive check in 3d2png and when the time comes to support other formats than STL, make the expected format of the source file a parameter of 3d2png.

wikimedia_thumbor/engine/stl/stl.py
90

The screen option should be -screen and the double quotes are wrong, they need to be removed.

MarkTraceur updated this revision to Diff 1956.Aug 8 2017, 6:36 PM

Fix temporary file name

Gilles added inline comments.Aug 8 2017, 7:49 PM
wikimedia_thumbor/engine/stl/stl.py
90

You still need to remove the double quotes here. Python takes care of the escaping on its own, and keeping them results in a syntax error.

Gilles added a comment.Aug 8 2017, 7:52 PM

The reference thumbnails are square. So it fails because the aspect ratio is different. I think you need to regenerate the reference thumbnails with the right dimensions. The aspect ratio in the Mediawiki extension is definitely 640 /480.

Gilles added inline comments.Aug 8 2017, 8:06 PM
wikimedia_thumbor/loader/swift/__init__.py
91–94

This needs to be moved below the logging.disable call. Being here masked this issue:

TypeError: descriptor 'lower' requires a 'str' object but received a 'unicode'

That code should just be:

extension = path[-4:].lower()
MarkTraceur updated this revision to Diff 1960.Aug 9 2017, 6:08 PM
MarkTraceur marked 3 inline comments as done.

Fix the previous problems

Gilles added a comment.Aug 9 2017, 6:36 PM

The reference thumbnails still have incorrect dimensions (square), which will make the tests fail.

Gilles updated this revision to Diff 1961.Aug 9 2017, 7:12 PM

Fix reference thumbnails dimensions

Gilles added inline comments.Aug 9 2017, 7:18 PM
wikimedia_thumbor/loader/swift/__init__.py
131

I've just realized that we forgot about this case where the file is smaller than the excerpt limit, in which case we don't use the temp file, and the buffer passed for MIME detection is also used for image processing.

In that case, for a binary STL, we're nuking actual content on line 117.

Gilles added inline comments.Aug 9 2017, 7:20 PM
wikimedia_thumbor/loader/swift/__init__.py
131

Ah, nevermind, for binary STLs the first 80 bytes are discarded, so modifying it is fine.

Gilles accepted this revision.Aug 9 2017, 7:20 PM
This revision is now accepted and ready to land.Aug 9 2017, 7:20 PM
MarkTraceur marked 2 inline comments as done.Aug 9 2017, 7:21 PM
Closed by commit rTHMBREXT69a3d6d63c9a: Add STL engine (authored by Mark Holmquist <mholmquist@wikimedia.org>, committed by Gilles). · Explain WhyAug 9 2017, 7:23 PM
This revision was automatically updated to reflect the committed changes.