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 camera video recorder system #316

Merged
merged 10 commits into from
Sep 15, 2020
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Aug 28, 2020

Add camera video recorder system that can be attached to a camera sensor and provides a service for recording a video from the sensor's image stream.

To test, launch the camera_video_record_dbl_pendulum.sdf world:

ign gazebo -v 4 -r ./src/ign-gazebo/examples/worlds/camera_video_record_dbl_pendulum.sdf

Call the following service to start video recording:

ign service -s /camera/record_video --reqtype ignition.msgs.VideoRecord --reptype ignition.msgs.Boolean --timeout 300 --req 'start: true, format:"mp4", save_filename:"test.mp4"'

Call the following service to stop video recording:

ign service -s /camera/record_video --reqtype ignition.msgs.VideoRecord --reptype ignition.msgs.Boolean --timeout 300 --req 'stop: true'

The video will be saved to test.mp4

@iche033 iche033 requested a review from nkoenig August 28, 2020 01:51
@iche033 iche033 requested a review from chapulina as a code owner August 28, 2020 01:51
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Aug 28, 2020
@iche033 iche033 force-pushed the camera_video_recorder branch from 2dce495 to ee796e5 Compare August 28, 2020 01:52
@nkoenig
Copy link
Contributor

nkoenig commented Aug 31, 2020

Works great. Just a few comments.

@iche033 iche033 force-pushed the camera_video_recorder branch from ee796e5 to d90f78e Compare August 31, 2020 23:47
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM.

Only nit would be: Is there a way to expose available video formats, or at least give a warning when there is an incorrect format?

@iche033 iche033 force-pushed the camera_video_recorder branch from 36323b5 to ed72b72 Compare September 1, 2020 23:11
@iche033
Copy link
Contributor Author

iche033 commented Sep 1, 2020

Only nit would be: Is there a way to expose available video formats, or at least give a warning when there is an incorrect format?

added check and print out error if format not supported, and also lists available formats. ed72b72

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

The new test is failing on Linux and on Mac:

/var/lib/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/ign-gazebo/test/integration/camera_video_record_system.cc:80
Value of: hasService
  Actual: false
Expected: true
53: [ RUN      ] CameraVideoRecorderTest.RecordVideo
53: [Wrn] [URI.cc:580] Unable to parse URI [/Users/jenkins/workspace/ignition_gazebo-ci-pr_any-homebrew-amd64/ign-gazebo/test/worlds/camera_video_record.sdf]. Ignoring.
53: objc[52149]: Class OgreConfigWindowDelegate is implemented in both /usr/local/opt/ogre1.9/lib/libOgreMain.1.9.0.dylib (0x116d86dc0) and /usr/local/opt/ogre2.1/lib/libOgreMain.2.1.0.dylib (0x1167af838). One of the two will be used. Which one is undefined.
53: dyld: lazy symbol binding failed: Symbol not found: __ZN4Ogre2v113OverlaySystemC1Ev
53:   Referenced from: /usr/local/Cellar/ignition-rendering3/3.1.0/lib/libignition-rendering3-ogre2.dylib
53:   Expected in: flat namespace
53: 
53: dyld: Symbol not found: __ZN4Ogre2v113OverlaySystemC1Ev
53:   Referenced from: /usr/local/Cellar/ignition-rendering3/3.1.0/lib/libignition-rendering3-ogre2.dylib
53:   Expected in: flat namespace
53: 
 53/138 Test  #53: INTEGRATION_camera_video_record_system ...............Child aborted***Exception:   1.18 sec

Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor Author

iche033 commented Sep 10, 2020

@osrf-jenkins run tests please

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #316 into ign-gazebo3 will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #316      +/-   ##
===============================================
+ Coverage        77.19%   77.24%   +0.05%     
===============================================
  Files              197      197              
  Lines            10479    10479              
===============================================
+ Hits              8089     8095       +6     
+ Misses            2390     2384       -6     
Impacted Files Coverage Δ
src/SimulationRunner.cc 93.59% <0.00%> (+1.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93565f0...3b89fa6. Read the comment docs.

@iche033
Copy link
Contributor Author

iche033 commented Sep 14, 2020

@osrf-jenkins run tests please

@iche033
Copy link
Contributor Author

iche033 commented Sep 15, 2020

latest CI builds look good. Failing tests on homebrew are unrelated.

@iche033 iche033 requested a review from chapulina September 15, 2020 04:05
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

The test failures have been fixed, so I can remove my request for changes. I haven't reviewed the PR, but the 2 approvals are enough.

@chapulina chapulina merged commit 4abd07c into ign-gazebo3 Sep 15, 2020
@chapulina chapulina deleted the camera_video_recorder branch September 15, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants