Update to the new Images #92

timholy · 2017-02-02T00:11:52Z

This performs a couple of cleanup operations, and then updates for recent changes in the FixedPointNumbers/Images landscape. Most significantly, it decouples Video from Images itself. In this version I have it relying on ImageCore (a much smaller package), but even that isn't really necessary: I think the only function I use from there is permuteddimsview, and that's basically a wrapper around Base.PermutedDimsArrays.PermutedDimsArray. (I use a little bit more functionality for the tests.) If you'd prefer to decouple entirely, that would be fine.

It does change the output type returned by read: rather than an Image that wraps an Array{UInt8}, it returns a (lazy-transposed, i.e., permuted) Array{RGB{N0f8}}. As a consequence, this is a breaking change.

This bumps the minimum julia version requirement, so if you like it this will need a minor-version bump when tagged.

There's logic here that is incompatible with precompilation. Better to prevent attempts to use it.

…ore)

timholy · 2017-02-02T03:13:56Z

The 0.5 stall is puzzling. Tests work for me locally, but since both instances failed it seems likely to be real. We could try running on Trusty, I suppose?

kmsquire · 2017-02-02T07:13:48Z

src/avio.jl

-    end
-end
-
-try
    if isa(Main.ImageView, Module)


I'd rather just remove this and put it in a separate package (although that doesn't have to happen in this PR).

kmsquire · 2017-02-02T07:15:21Z

@timholy, thanks for this! I've been planning to try to decouple Images (and ImageView) for a long time, but it's taken the back burner.

I'll take a closer look at this tomorrow or Friday.

timholy · 2017-02-02T09:42:54Z

I suspect that if we make it separate from ImageView, too, then we can turn on precompilation.

For reference:

Neither of those is necessary for this, but in the future it can become a little cleaner.

tkelman · 2017-02-18T01:49:14Z

How's this coming?

It looks like Compat is used in quite a few more places that aren't all touched here, which may need to be if it's being removed from REQUIRE.

kmsquire · 2017-02-18T14:28:34Z

test/avio.jl

-    img = read(v, Image)
+    if size(first_frame, 1) > v.height
+        first_frame = first_frame[1+size(first_frame,1)-v.height:end,:]
+    end


I'm curious what necessitated this change? Tests pass for me without it (using ImageMagick on a Mac), and the images being tested against were pulled directly from the videos that are being read, so they should be exactly the same size.

My libav inserted an extra row compared to the reference image.

kmsquire · 2017-02-18T14:29:08Z

test/avio.jl

-    img = read(v, Image)
+    if size(first_frame, 1) > v.height
+        first_frame = first_frame[1+size(first_frame,1)-v.height:end,:]
+    end


Same here, of course.

kmsquire · 2017-02-18T14:58:54Z

For the failing v0.5 tests, the video reading just seems incredibly slow. One way to get the tests to pass would be to print something every 100 frames or shorten the total number of frames being read.

timholy · 2017-02-18T16:10:57Z

Hmm. I developed this on 0.5, and for me they aren't slow:

$ time julia-0.5 runtests.jl 
VideoFile:
   name:         annie_oakley.ogg  (Downloaded)
   description:  The "Little Sure Shot" of the "Wild West," exhibition of rifle shooting at glass balls.
   license:      pubic_domain (US)
   credit:       
   source:       http://commons.wikimedia.org/wiki/File:Annie_Oakley_shooting_glass_balls,_1894.ogg
   download_url: https://upload.wikimedia.org/wikipedia/commons/8/87/Annie_Oakley_shooting_glass_balls%2C_1894.ogv
 
VideoFile:
   name:         crescent-moon.ogv  (Downloaded)
   description:  Moonset (time-lapse).
   license:      Creative Commons Attribution 2.0 Generic (http://creativecommons.org/licenses/by/2.0/deed)
   credit:       Photo : Thomas Bresson
   source:       http://commons.wikimedia.org/wiki/File:2010-10-10-Lune.ogv
   download_url: http://upload.wikimedia.org/wikipedia/commons/e/ef/2010-10-10-Lune.ogv
 
VideoFile:
   name:         ladybird.mp4  (Downloaded)
   description:  Ladybird opening wings (slow motion)
   license:      Creative Commons: By Attribution 3.0 Unported (http://creativecommons.org/licenses/by/3.0/deed)
   credit:       CC-BY NatureClip (http://youtube.com/natureclip)
   source:       http://downloadnatureclip.blogspot.com/p/download-links.html
   download_url: https://archive.org/download/LadybirdOpeningWingsCCBYNatureClip/Ladybird%20opening%20wings%20CC-BY%20NatureClip.mp4
 
VideoFile:
   name:         black_hole.webm  (Downloaded)
   description:  Artist’s impression of the black hole inside NGC 300 X-1 (ESO 1004c)
   license:      Creative Commons Attribution 3.0 Unported (http://creativecommons.org/licenses/by/3.0/deed)
   credit:       Credit: ESO/L. Calçada
   source:       http://www.eso.org/public/videos/eso1004a/
   download_url: http://upload.wikimedia.org/wikipedia/commons/1/13/Artist%E2%80%99s_impression_of_the_black_hole_inside_NGC_300_X-1_%28ESO_1004c%29.webm
 
Testing file reading...
   Testing annie_oakley.ogg...
   Testing crescent-moon.ogv...
   Testing ladybird.mp4...
   Testing black_hole.webm...
Testing IO reading...
   Testing annie_oakley.ogg...
   Testing crescent-moon.ogv...
   Testing black_hole.webm...

real    0m26.357s
user    0m37.076s
sys     0m0.720s

Can you profile it to see where the problem is?

I'm traveling soon so I won't be in frequent touch.

kmsquire · 2017-02-18T18:45:08Z

(Testing this idea now in kms/teh/newimages...)

kmsquire · 2017-02-18T18:47:14Z

Sorry @timholy, I just saw your last comment.

I think they might just be slow on travis machines. They're definitely not slow on a Mac either.

kmsquire · 2017-02-19T04:53:01Z

So far, tests just seem to hang when reading mp4 files. At least, none of the print statements I added work. The correct libraries seem to be installed....

Anyway, I'm going to try to reproduce locally using the travis docker images.

kmsquire · 2017-02-19T05:46:29Z

Turns out that --bounds-check=yes is the culprit for the timeout on Travis. If I have time, I'll explore further, but for now, I'm just going to turn it off.
It's a bit unfortunate that we won't be checking bounds, but if the tests won't run with it...

* Needs to be examined further, but it was responsible for a timeout on Julia v0.5

kmsquire · 2017-02-19T18:06:13Z

And... that didn't work. :-( At a little bit of a loss, but will try to investigate more as I have time.

kmsquire · 2017-02-21T18:04:25Z

A little more exploration: it turns out the slowdown (on Travis machines) is in notblank().

I attempted to rewrite the maps of anonymous functions using loops, but that didn't help. ☹️

@timholy, any thoughts? I'll attempt to look more into this when I have time, but not sure when.

timholy · 2017-02-21T19:44:01Z

I'm having trouble understanding how big of a slowdown you're talking about. One possible way to diagnose would be to disable one or the other of several tests, e.g., test only the green color channel or something.

tkelman · 2017-02-21T19:48:15Z

if it's just hitting the 10 minutes with no output thing on travis, there is a travis_wait command you can use - https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

For reasons I don't understand, turning on coverage causes the tests to take much longer, specifically in notblank. Since we're not doing anything with the coverage data anyway, let's just turn it off.

timholy · 2017-02-21T20:11:54Z

I was able to replicate the problem locally with Pkg.test("VideoIO", coverage=true). Turning off coverage fixes the problem (locally at least). Weird. I don't have time to dig deeper, so disabling coverage seems like the expedient solution.

timholy · 2017-02-21T20:41:42Z

Looks like that worked. There's an error on 0.6,

UndefVarError: __va_list_tag not defined

which comes from one of the libav log.jl files. I wasn't able to find any place where this variable was defined, so I'm not quite sure how to go about fixing it.

But at least everything works on 0.5.

kmsquire · 2017-02-22T06:46:53Z

I'm having trouble understanding how big of a slowdown you're talking about. One possible way to diagnose would be to disable one or the other of several tests, e.g., test only the green color channel or something.

It was taking over 2 minutes to test whether a frame was blank! I also had thought about testing in the way you said, but I honestly just didn't have time.

if it's just hitting the 10 minutes with no output thing on travis, there is a travis_wait command you can use - https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

While debugging, I was able to output enough debug statements that this wasn't a problem. Instead, it was running over the total time allotted.

Turning off coverage fixes the problem (locally at least). Weird. I don't have time to dig deeper, so disabling coverage seems like the expedient solution.

Looks like that worked. There's an error on 0.6,

Thanks! The v0.6 error shouldn't be that hard to fix (it's caused by trying to let some C vararg functions be wrapped... but really, there's no real reason to be wrapping the libav log.jl file anyway.)

kmsquire · 2017-02-22T07:46:26Z

...but really, there's no real reason to be wrapping the libav log.jl file anyway

Except if you want to set the log level to reduce the number of warning messages libav/ffmpeg write.

Anyway, I have some easy fixes for this and a few other cleanups that I'll push in a few minutes.

timholy added 5 commits February 1, 2017 18:03

Turn off precompilation explicitly

a9b1bac

There's logic here that is incompatible with precompilation. Better to prevent attempts to use it.

Update link for Annie Oakley

54dcb06

Update to the new Images (and decouple from anything more than ImageC…

7667984

…ore)

gitignore videos/

079fafc

Travis-test on julia 0.5

576122d

kmsquire reviewed Feb 2, 2017

View reviewed changes

kmsquire mentioned this pull request Feb 18, 2017

Column first or Row first? JuliaIO/QuartzImageIO.jl#31

Closed

kmsquire reviewed Feb 18, 2017

View reviewed changes

Don't check bounds on travis

55217c5

* Needs to be examined further, but it was responsible for a timeout on Julia v0.5

kmsquire force-pushed the teh/newimages branch from 1d62008 to 55217c5 Compare February 19, 2017 05:52

Prevent inlining in notblank and turn off coverage

1531d08

For reasons I don't understand, turning on coverage causes the tests to take much longer, specifically in notblank. Since we're not doing anything with the coverage data anyway, let's just turn it off.

timholy force-pushed the teh/newimages branch from 6253881 to 1531d08 Compare February 21, 2017 20:10

kmsquire merged commit e019929 into master Feb 22, 2017

kmsquire deleted the teh/newimages branch February 22, 2017 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to the new Images #92

Update to the new Images #92

timholy commented Feb 2, 2017

timholy commented Feb 2, 2017

kmsquire Feb 2, 2017

SimonDanisch Feb 2, 2017

kmsquire commented Feb 2, 2017

timholy commented Feb 2, 2017

tkelman commented Feb 18, 2017

kmsquire Feb 18, 2017

timholy Feb 18, 2017

kmsquire Feb 18, 2017

kmsquire commented Feb 18, 2017

timholy commented Feb 18, 2017

kmsquire commented Feb 18, 2017

kmsquire commented Feb 18, 2017

kmsquire commented Feb 19, 2017

kmsquire commented Feb 19, 2017

kmsquire commented Feb 19, 2017

kmsquire commented Feb 21, 2017

timholy commented Feb 21, 2017

tkelman commented Feb 21, 2017

timholy commented Feb 21, 2017

timholy commented Feb 21, 2017

kmsquire commented Feb 22, 2017

kmsquire commented Feb 22, 2017

Update to the new Images #92

Update to the new Images #92

Conversation

timholy commented Feb 2, 2017

timholy commented Feb 2, 2017

kmsquire Feb 2, 2017

Choose a reason for hiding this comment

SimonDanisch Feb 2, 2017

Choose a reason for hiding this comment

kmsquire commented Feb 2, 2017

timholy commented Feb 2, 2017

tkelman commented Feb 18, 2017

kmsquire Feb 18, 2017

Choose a reason for hiding this comment

timholy Feb 18, 2017

Choose a reason for hiding this comment

kmsquire Feb 18, 2017

Choose a reason for hiding this comment

kmsquire commented Feb 18, 2017

timholy commented Feb 18, 2017

kmsquire commented Feb 18, 2017

kmsquire commented Feb 18, 2017

kmsquire commented Feb 19, 2017

kmsquire commented Feb 19, 2017

kmsquire commented Feb 19, 2017

kmsquire commented Feb 21, 2017

timholy commented Feb 21, 2017

tkelman commented Feb 21, 2017

timholy commented Feb 21, 2017

timholy commented Feb 21, 2017

kmsquire commented Feb 22, 2017

kmsquire commented Feb 22, 2017