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.

Sign up for GitHub

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

Jump to bottom

Fix a typo in VideoEncoder_TEST. #231

Merged
merged 3 commits into from
Jul 28, 2021
Merged

Fix a typo in VideoEncoder_TEST. #231

merged 3 commits into from
Jul 28, 2021

Conversation

mahiuchun
Copy link
Contributor

The same typo is also in ign-common4 and main branches.

@mahiuchun mahiuchun requested a review from mjcarroll as a code owner July 21, 2021 19:47
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress labels Jul 21, 2021
@mahiuchun mahiuchun changed the base branch from ign-common4 to ign-common3 July 21, 2021 19:47
@mahiuchun
Copy link
Contributor Author

Looks like mpg rather than mp4 passes in the CI, is this the right behavior though?

@scpeters
Copy link
Member

Looks like mpg rather than mp4 passes in the CI, is this the right behavior though?

I see why you made this change, but it causes the test to fail. I don't understand the code well enough to say why

@chapulina chapulina added the AV AV component label Jul 22, 2021
@mjcarroll
Copy link
Contributor

I see why you made this change, but it causes the test to fail. I don't understand the code well enough to say why

I agree that the change is correct, but the failure is very strange.

@mjcarroll
Copy link
Contributor

The issue here was that the test was calling Start twice without a Stop or Reset in between the two Start calls. I have added some more comprehensive tests to test default, mpg, and mp4 encoding, as well as testing the multiple Start case.

@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #231 (887e2dc) into ign-common3 (1c1eec8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-common3     #231   +/-   ##
============================================
  Coverage        76.55%   76.55%           
============================================
  Files               73       73           
  Lines            10386    10386           
============================================
  Hits              7951     7951           
  Misses            2435     2435

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 1c1eec8...887e2dc. Read the comment docs.

@chapulina chapulina merged commit 3bff8ac into gazebosim:ign-common3 Jul 28, 2021
scpeters pushed a commit to scpeters/ign-common that referenced this pull request Jan 19, 2022
@luca-della-vedova @mjcarroll
Merged in pbr_common3 (pull request gazebosim#231)
ea290c2 
Set default PBR metalness value

Approved-by: Louise Poubel <[email protected]>
Approved-by: Luca Della Vedova <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll

Assignees
No one assigned
Labels
AV AV component 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mahiuchun @scpeters @mjcarroll @chapulina