Skip to content
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

Add FindFrames command #110

Merged
merged 4 commits into from
Jul 10, 2019
Merged

Add FindFrames command #110

merged 4 commits into from
Jul 10, 2019

Conversation

mahircg
Copy link
Contributor

@mahircg mahircg commented Jun 21, 2019

This patch introduces 'FindFrames' command, that searches for the given
video name and returns a list of frames from the video if it is found.

Currently, FindFrame command relies on OpenCV's frame iteration
capabilities. Thus, the entire video is loaded into memory to be
decoded. In future, the video loading and decoding should be done only
on the requested frame interval.

Signed-off-by: mahircg [email protected]

@vishakha041
Copy link
Contributor

I'll look at this once we close the other PR. I believe this is based off of that one.

@luisremis
Copy link
Contributor

@vishakha041, these are independent, as you can see following the commits. So you can go ahead and review it.

Copy link
Contributor

@vishakha041 vishakha041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to do the Operator change before this one so you can have a cleaner version of FindFrames and remove your comment about the hack.

This is a quick review for some things that were noticeable.

src/VideoCommand.cc Outdated Show resolved Hide resolved
src/VideoCommand.cc Show resolved Hide resolved
src/VideoCommand.cc Outdated Show resolved Hide resolved
src/VideoCommand.cc Outdated Show resolved Hide resolved
src/VideoCommand.cc Show resolved Hide resolved
src/VideoCommand.cc Show resolved Hide resolved
src/VideoCommand.cc Outdated Show resolved Hide resolved
src/VideoCommand.cc Outdated Show resolved Hide resolved
tests/python/TestVideos.py Show resolved Hide resolved
utils/src/api_schema/api_schema.json Show resolved Hide resolved
@luisremis luisremis force-pushed the find_frame branch 3 times, most recently from ffc3f81 to 694e7ef Compare June 28, 2019 21:26
@luisremis
Copy link
Contributor

All comments addressed. Good to go for me!

@mahircg
Copy link
Contributor Author

mahircg commented Jul 1, 2019

I've just did an interactive rebase on all three patches to sign them off. All the changes look good to me.

luisremis
luisremis previously approved these changes Jul 1, 2019
Copy link
Contributor

@luisremis luisremis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@luisremis
Copy link
Contributor

luisremis commented Jul 2, 2019

Created a separate commit for fixing a bug in Video command response.

Should be good to go.

@luisremis luisremis dismissed vishakha041’s stale review July 9, 2019 02:34

All comments addressed

@luisremis luisremis self-requested a review July 9, 2019 02:34
Copy link
Contributor

@vishakha041 vishakha041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the comments do not indicate a problem, then this looks fine.

@luisremis luisremis merged commit 078951e into develop Jul 10, 2019
@luisremis luisremis deleted the find_frame branch July 10, 2019 16:06
cwlacewe added a commit to cwlacewe/vdms that referenced this pull request Apr 14, 2023
* Fix CIDs 3428250, 3399517, 3399721
* Fix CID 3399499
* Fix CIDs 3399722 and 3399506
* Fix CID 3399609 and 3399442
* Fix CID 3399441
* Fix 3399465 and 3399606
* fix 3399466
* Fix CID 3399404 and 3399679
* Fix CID 3441993
Signed-off-by: tmcourie <[email protected]>
Co-authored-by: tmcourie <[email protected]>
Co-authored-by: Ragaad <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants