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

Now reading videos using subprocesses and ffmpeg. #702

Merged
merged 24 commits into from Jun 7, 2016

Conversation

Projects
None yet
4 participants
@JeanKossaifi
Contributor

JeanKossaifi commented May 26, 2016

No description provided.

@jabooth jabooth added the in progress label May 26, 2016

@JeanKossaifi JeanKossaifi changed the title from Now reading videos using subprocessed and ffmpeg. to Now reading videos using subprocesses and ffmpeg. May 26, 2016

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented May 26, 2016

To test it locally, make sure nose and mock are installed and then run nosetests menpo -v in order to see which tests are failing.

Also, would be great if you could just sign the CLA for now - currently we have that to protect us from anything regarding issues with copyright in the future.


infos = {'duration': duration, 'width': width, 'height': height, 'n_frames': n_frames, 'fps': fps}

return infos

This comment has been minimized.

@jabooth

JeanKossaifi and others added some commits May 26, 2016

Added ffmpeg exporter, refactored roll_channels.
channels_to_back is now roll_channels and menpo images have a
as_roll_channels method to replace the now deprecated as_imageio.
Update how ffprobe is parsed
Also, add special environment variables for ffmpeg and ffprobe
so that the user can update them.

Finally, add mpg and mpeg to list of supported formats.
Deprecate rather than remove rolled_channels
Remove roll_channels as I don't think it's necessary and it's
name is confusing when compared to as_rolled_channels.
as_rolled_channels allow setting out_dtype
Makes denormalising easier.
Stop using imageio - custom ffmpeg
Remove imageio completely from setup.py and conda/meta.yaml
and improve the ffmpeg logic a bit. Since we are no longer using
imageio this also meant fixing some tests.

Also, try and make sure that our pipe handling is more robust.
Merge branch 'master' into video_importer
Conflicts:
	menpo/io/input/video.py
	menpo/io/test/io_import_test.py
Merge branch 'master' of https://github.com/menpo/menpo into video_im…
…porter

Conflicts:
	menpo/io/input/video.py
	menpo/io/test/io_import_test.py
Merge pull request #2 from patricksnape/video_importer
Merge commit from Greg's normalize keyword change
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 1, 2016

@jabooth - @JeanKossaifi think this is ready now - can you please check it? Also, would be great if you actually loaded some videos. @grigorisg9gr it would also be great if you could check it with some of your videos?

A dictionary of extensions supported by the FFMPEG importer.
"""
# Need to check this
ffmpeg_exts = ['.avi', '.mp4', '.mpg', '.mpeg', '.wmv', '.mov']

This comment has been minimized.

@grigorisg9gr

grigorisg9gr Jun 1, 2016

Member

I would definitely extend that to include more. E.g. mkv is quite common.
You can find the supported ones with ffmpeg -formats.

This comment has been minimized.

@patricksnape

patricksnape Jun 2, 2016

Contributor

I can add mkv, but my major concern is that many of the formats listed by ffmpeg -formats only work if ffmpeg has been compiled with all of it's extensions - not everything is supported out the box. So, there's a part of me that only wants to support those formats that are very familiar and only add other formats as requested and confirmed that they actually work with our shipped ffmpeg.

This comment has been minimized.

@JeanKossaifi

JeanKossaifi Jun 2, 2016

Contributor

+1
That is also the reason why I didn't add list them in the first place.

This comment has been minimized.

@patricksnape

patricksnape Jun 2, 2016

Contributor

@JeanKossaifi can you add mkv?

This comment has been minimized.

@JeanKossaifi
@grigorisg9gr

This comment has been minimized.

Member

grigorisg9gr commented Jun 1, 2016

I tested it in a couple of videos. It works ok I believe. Nice work @JeanKossaifi and @patricksnape .

Even in some videos of bigger duration it works without consuming much ram, however it might require some time.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 6, 2016

@JeanKossaifi L744/45 in input/base.py, can you please add below new_ll.path = x.path to ensure that the imported video has the path attached? This should sort out #705 for @grigorisg9gr.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 6, 2016

@jabooth Please review :)

MenpoDeprecationWarning)
return self.as_rolled_channels()

def as_rolled_channels(self, out_dtype=None):

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

completely support deprecating as_imageio but not sure as_rolled_channels() is the right name here, and if we are deprecating, might be worth a little discussion on getting it right.

For my money, that name to me doesn't suggest I'm going to get a numpy array back, so I'd assume an Image is the return type. That then makes me think that maybe menpo images support channels either rolled or not, which is not the case.

My other preference would be to move from channels_rolled generally to channels_at_front or channels_at_back respectfully. Rolling is the procedure to change from one to another, but it isn't clear to the callee what the input and output should be.

Those two thoughts lead me to suggest a name more like .pixels_with_channels_at_back() or even .pixels_array_with_channels_at_back(). That's obviously super verbose though, can we try and find something shorter than that that still conveys:

  1. You are getting back a numpy array only
  2. The channels on the array are at the back so you can use this array with code that expects that convention
  3. Re-enfoces that menpo images must therefore always have channels not at the back, but the front.

Thoughts @patricksnape ?

This comment has been minimized.

@patricksnape

patricksnape Jun 6, 2016

Contributor

Yes, to be fair, I've struggled with the naming convention of this the entire time. There is no standard term for where the channels lie and so everything might seem confusing to someone. Rolling is obviously what you actually do to move the axis. 'back' and 'front' are more obvious, but might be confusing to someone with regards to the 'back/front' of what? 'First' and 'last' axis are more explicit but lend themselves to longer names. It's tough, discoverability is important but super verbose method names for fairly common functions are also undesirable.

.pixels_array_with_channels_at_back() is definitely too long. I don't think we need to specify array as we already have the convention that pixels gives access to the underlying numpy array. So I would vote for .pixels_with_channels_at_back().

I can see why the as_ syntax might suggest returning a Menpo object given the definition of similar methods. However, obviously this needs to be a method as it involves a copy. We also need to fix the init_from_rolled_channels to have consistent naming e.g. init_from_channels_at_back().

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

Totally agree with your opening paragraph.

.pixels_array_with_channels_at_back() is definitely too long. I don't think we need to specify array as we already have the convention that pixels gives access to the underlying numpy array.

Yup, had a similar thought with the .pixels convention already existing

So I would vote for .pixels_with_channels_at_back().

Agreed. Also has the nice benefit that it will appear when tab completing .pixels, aiding discoverability.

However, obviously this needs to be a method as it involves a copy.

Agreed.

We also need to fix the init_from_rolled_channels to have consistent naming e.g. init_from_channels_at_back().

Yup, agreed. This feels like an overall better solution to a really tricky naming issue.

This comment has been minimized.

@JeanKossaifi

JeanKossaifi Jun 6, 2016

Contributor

What about simply rolled_channels_ndarray?

On 6 June 2016 at 13:24, James Booth notifications@github.com wrote:

In menpo/image/base.py
#702 (comment):

@@ -2525,17 +2530,38 @@ def pixels_range(self):

 def rolled_channels(self):
     r"""
  •    Deprecated - please use the equivalent `as_rolled_channels` method.
    
  •    """
    
  •    warn('This method is no longer supported and will be removed in a '
    
  •         'future version of Menpo. '
    
  •         'Use .as_rolled_channels() instead.',
    
  •         MenpoDeprecationWarning)
    
  •    return self.as_rolled_channels()
    
  • def as_rolled_channels(self, out_dtype=None):

Totally agree with your opening paragraph.

.pixels_array_with_channels_at_back() is definitely too long. I don't
think we need to specify array as we already have the convention that
pixels gives access to the underlying numpy array.

Yup, had a similar thought with the .pixels convention already existing

So I would vote for .pixels_with_channels_at_back().

Agreed. Also has the nice benefit that it will appear when tab completing
.pixels, aiding discoverability.

However, obviously this needs to be a method as it involves a copy.

Agreed.

We also need to fix the init_from_rolled_channels to have consistent
naming e.g. init_from_channels_at_back().

Yup, agreed. This feels like an overall better solution to a really tricky
naming issue.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/menpo/menpo/pull/702/files/56bf939db7eb788371c54563876feaaf90bc5a3f#r65880732,
or mute the thread
https://github.com/notifications/unsubscribe/AAy0R5GQgndD5V2PoPxjfutQ2RHyHGhdks5qJBFpgaJpZM4Ine8W
.

This comment has been minimized.

@patricksnape

patricksnape Jun 6, 2016

Contributor

I like the discoverability provided with the pixels name to be honest.

This comment has been minimized.

@JeanKossaifi

JeanKossaifi Jun 6, 2016

Contributor

The name .pixels_with_channels_at_back() seems overly verbose.
What about, instead, making a generic getter function with a default parameter encouraging the behaviour you want (channels at the front), e.g. .pixels_to_array(channels_at_end=False)?

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

@JeanKossaifi problem with that getter is that

.pixels_to_array(channels_at_end=False)

is just going to hand you the pixels array which is

.pixels

It's an important convention in menpo that our objects are minimal wrappers with obvious access to the underlying numpy array, so .pixels is and will remain the supported way of getting hold of the pixel array.

.pixels_with_channels_at_back() is inline with existing Menpo naming conventions. It might be considered long for a Python method name, but we tend to follow the Apple school of thought on this issue:

It is good to be both clear and brief as possible, but clarity shouldn’t suffer because of brevity

People using Menpo will undoubtedly tab-complete names - why make it harder to discover what a method does in the name of optimising for source code size/typing names without assistance?

This comment has been minimized.

@JeanKossaifi

JeanKossaifi Jun 6, 2016

Contributor

I see your point and if that is the consensus I'll change it to .pixels_with_channels_at_back.

However, if the point of the object is just to have a thin wrapper around a ndarray representing an image, a .to_array(channels_at_back=False) would be as coherent.

image.to_array() has the expected behaviour and image.to_array(channels_at_back=True) is very clear. The advantage being the possible addition of other optional parameters such as normalise etc. Also, in practice, relying solely on users tab-completing is not very robust and can result in an overly long list. Again, just a suggestion.

In any case, I updated the PR to include the other comments so unless there are other comments, the it should be good to merge once that naming issue is solved :)

video in to memory which may cause a delay in loading despite the lazy
nature of the video loading within Menpo. If ffprobe cannot be found,
Menpo falls back to ffmpeg itself which is not accurate and the user
should proceed at their own risk.

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

If we feel this could be a real problem (and I could really see where it could screw you over if you are loading many sequences in correspondence and need them to be exactly the same frame count) maybe we should add a kwarg that allows you to opt out of the ffmpeg fallback. Maybe something like strict_frame_count=False, so not suggesting a change in default behaviour, but you can invoke with strict_frame_count=True and then you will just get a ValueError bubbled up if ffprobe not found?

This comment has been minimized.

@patricksnape

patricksnape Jun 6, 2016

Contributor

Yeah sure, I can support the idea of that kwarg.

video in to memory which may cause a delay in loading despite the lazy
nature of the video loading within Menpo. If ffprobe cannot be found,
Menpo falls back to ffmpeg itself which is not accurate and the user
should proceed at their own risk.

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

Same kwarg here



def imageio_ffmpeg_importer(filepath, asset=None, normalize=True, **kwargs):
def ffmpeg_importer(filepath, normalize=True, **kwargs):
r"""
Imports videos using the FFMPEG plugin from the imageio library. Returns a

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

Is this docstring accurate, referring to imageio?

[imageio_ffmpeg_importer] * len(ffmpeg_exts)))
except ImportError:
return {}
_FFMPEG_CMD = lambda: str(Path(os.environ.get('MENPO_FFMPEG_CMD', 'ffmpeg')))

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

Am I misreading this, or does this now require that MENPO_FFMPEG_CMD must be set for video IO to work? I figured it would be a fallback to customise?

This comment has been minimized.

@patricksnape

patricksnape Jun 6, 2016

Contributor

You are misreading it. The get makes the default ffmpeg which allows PATH resolution to find the correct binary.

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

I'll blame the stag do for that one... 😮

for stream in (process.stdout, process.stdin, process.stderr):
if stream:
stream.close()
process.wait()

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

very nice 👍

warnings.warn('Estimating number of frames using ffmpeg duration which '
'may be inaccurate for certain types of encodings. Try '
'setting the MENPO_FFPROBE_CMD environment variable to '
'define the path to ffprobe.')

This comment has been minimized.

@jabooth

jabooth Jun 6, 2016

Member

Ah fair enough, with this my kwarg suggestion above is probably redundant.
Actually, the kwarg still has value -better to be explicit than implicit.

@JeanKossaifi

This comment has been minimized.

Contributor

JeanKossaifi commented Jun 6, 2016

@jabooth @patricksnape should be ready to merge:

  • changed rolled_channels to pixels_with_channels_at_back
  • changed the class method init_from_rolled_channels to init_from_channels_at_back
  • exact_frame_count is now True by default (i.e. will fail is ffprobe is not present, to default to ffmpeg, has to be explicitly set to False). If ffprobe if not present and exact_frame_count is True, a ValueError will be raised with an appropriate informative message.
@@ -125,7 +125,7 @@ def init_blank(cls, shape, n_channels=1, fill=0, dtype=np.float, mask=None):
return cls(pixels, copy=False, mask=mask)

@classmethod
def init_from_rolled_channels(cls, pixels, mask=None):
def init_from_channels_at_back(cls, pixels, mask=None):

This comment has been minimized.

@patricksnape

patricksnape Jun 6, 2016

Contributor

@JeanKossaifi Can we deprecate this rather than just replacing it? So re-add it and deprecate it in the same manner as rolled_channels above (but obviously referencing init_from_channels_at_back). The tests are correct though.

This comment has been minimized.

@JeanKossaifi

JeanKossaifi Jun 6, 2016

Contributor

This is already what I did, at least from what I see, I think the gittiff is just confusing in that case.

This comment has been minimized.

@patricksnape

patricksnape Jun 6, 2016

Contributor

My bad! Let me check the code out and check 😆
Edit: Confirmed - apologies!

@@ -167,7 +167,7 @@ def denormalize_pixels_range(pixels, out_dtype):
return (pixels * max_range).astype(out_dtype)


def channels_to_back(pixels):
def roll_channels(pixels):

This comment has been minimized.

@patricksnape

patricksnape Jun 7, 2016

Contributor

@JeanKossaifi I promise. Last thing 😆 ! Since we settled on channels_to_back can we keep this as channels_to_back?

This comment has been minimized.

@JeanKossaifi

JeanKossaifi Jun 7, 2016

Contributor

done.

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 7, 2016

@jabooth Once this finished I am good to go on this. @JeanKossaifi thanks for your hard work!

@jabooth

This comment has been minimized.

Member

jabooth commented Jun 7, 2016

@patricksnape looks good to me, thanks so much @JeanKossaifi! +1 once passed.

@JeanKossaifi

This comment has been minimized.

Contributor

JeanKossaifi commented Jun 7, 2016

Thanks for the comments @jabooth and @patricksnape. Not sure what happened with appveyor here, it does not seem to be a problem in the code?

One last thing: currently I made the FFMpegVideoReader return Menpo Image.
I realise it might make sense to make it more generic, returning numpy arrays and move the conversion (which has to be done anyway) in the video_importer?

Or alternatively add an optional parameter convert_to_menpo set to True by default? This is transparent to the end user (if they use the menpo video_importer they would not have to worry about it) and it would allow advanced users to instantiate directly a FFMpegVideoReader to get channels_at_the_back images directly rather than do the conversion twice :)

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 7, 2016

I restarted Appveyor - it was a capacity failure on the Anaconda server.

I would prefer, if you really want to do that, that you let your class return the numpy arrays and then you do the conversion at the level of the ffmpeg_importer method rather than any keyword arguments. So something like

# This reader will return channel_at_the_back numpy arrays, possibly normalized
reader = FFMpegVideoReader(filepath, normalize=normalize, exact_frame_count=exact_frame_count)
ll = LazyList.init_from_index_callable(lambda x: Image.init_from_channels_at_back(reader[x])), len(reader))

Note that I haven't actually tried that logic, but I think it illustrates my point. That way you can always instantiate your own FFMpegVideoReader and read away but you lose all the rest of the Menpo machinery and you have to be digging into Menpo to find you can do that. I don't want to promote people bypassing Menpo types 😆.

Moved conversion into Image in video_importer.
The FFMpegVideoReader simply returns numpy arrays, the importer
initialises the Menpo Image with init_from_channels_at_back.
@JeanKossaifi

This comment has been minimized.

Contributor

JeanKossaifi commented Jun 7, 2016

Done. Completely transparent for the user (does not change a thing for importing videos) and they would have to dig to find out as you mentioned :)

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Jun 7, 2016

Now just the long wait for Appveyor and then LGTM.

@patricksnape patricksnape merged commit a1a11dc into menpo:master Jun 7, 2016

3 checks passed

OS X MenpoBot Jenkins build passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment