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

Video decoder fixes #114

Closed
wants to merge 8 commits into from
Closed

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Oct 30, 2020

This PR fixes a few things regarding video decoding. Descriptions of the respective fixed problems is in the commit messages.

Basically, without this PR, decoding an MP4 video ends either with a segfault or nonsense output. I wonder if it ever worked for someone before.

The output pixel format fix assumes that the desired format of the output buffer is R8G8B8 (that was deduced from the swscale config). But as the docs were scarce here, I'm not sure whether it wasn't desired to output YUV420.

Finally, I added a test that checks a decode-encode-decode loop for basic functionality. It seems that up to FPS frames get lost at the end of the encoded video, but the same was observed in #110 and accepted, so I also accept it here.

@peci1 peci1 requested a review from mjcarroll as a code owner October 30, 2020 02:33
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Oct 30, 2020
peci1 added 5 commits October 30, 2020 03:51
…s on success.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
The previous approach stopped working because libavcodec now probably also examines other members of the packet than size and data.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
We definitely want to output an R8G8B8 image (I guess that's the only reason for the swscale to be there).

Without this fix, the resulting image is a complete mess when interpreted as R8G8B8.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1 peci1 force-pushed the fix_video_decoder branch from 0ba90df to 8d1721a Compare October 30, 2020 02:58
@peci1
Copy link
Contributor Author

peci1 commented Oct 30, 2020

I manually tested compatibility of this PR with #105 and it ran ok.

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.

Small nits.

test/integration/video_encoder.cc Outdated Show resolved Hide resolved
test/integration/video_encoder.cc Show resolved Hide resolved
@mjcarroll
Copy link
Contributor

I believe that you'll also need to update the branch with latest ign-common3 changes as well as do a DCO signoff.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1 peci1 force-pushed the fix_video_decoder branch from 63ae6b5 to ac496fd Compare November 2, 2020 21:36
@peci1
Copy link
Contributor Author

peci1 commented Nov 11, 2020

Closing in favor of #125 which contains all of the fixes that were introduced in this PR.

@peci1 peci1 closed this Nov 11, 2020
@peci1 peci1 deleted the fix_video_decoder branch November 17, 2020 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants