search

JuliaIO / VideoIO.jl

Reading and writing of video files in Julia via ffmpeg
https://juliaio.github.io/VideoIO.jl/stable
Other
128 stars 53 forks source link

Add `ColorTypes.gray` pixels #145

Closed IanButterworth closed 5 years ago

IanButterworth commented 5 years ago

Hopefully a simple change to enable grayscale UInt8 based videos.

Also added an example for reading in a full video

kmsquire commented 5 years ago

Presumably you enabled this because you have a black and white video that you're trying to read.

Has this change been sufficient? It looks fine to me.

Is there a short black-and-white video that we might be able to test with? That might require changing how we get the test videos.

IanButterworth commented 5 years ago

It works for my local videos. How about this for a test video:

https://commons.wikimedia.org/wiki/File:Fluorescent-Protein-Stabilization-and-High-Resolution-Imaging-of-Cleared-Intact-Mouse-Brains-pone.0124650.s009.ogv

Perhaps the best way to handle this is to just make readvideo() read ColorTypes.gray for grayscale source videos? That way the testing method wouldn't need to change.

kmsquire commented 5 years ago

That test video seems fine!

Perhaps the best way to handle this is to just make readvideo() read ColorTypes.gray for grayscale source videos?

Assuming you mean read (and retrieve, which should probably just be renamed to read).
I was going to write something about type stability, but most of the code is already type unstable, so I guess that wouldn't change much.

However, I think it would be better to allow the user to specify the desired ColorType of the output, and map those to ffmpeg's pixel formats, which would allow the function to be type stable. We could still have a type-unstable version which first detected the pixel format of the input, and returned the most appropriate output format.

Those tasks are disjoint, so... that's a long way of saying, sure, we can just change read to return ColorTypes.gray for grayscale videos. ;-)

That way the testing method wouldn't need to change.

The tests (like many other parts of this package) could use some love. The basic ideas are fine, but the code itself should be reorganized. However, that doesn't have to be done here.

I'm going to create a some issues for the ideas above.

IanButterworth commented 5 years ago

@kmsquire I've not got around to doing the suggested test improvements. Do you think it would be ok to merge this before that happens?

kmsquire commented 5 years ago

Sure.