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

ffmpeg: Set audio channel count on AVFrame #20678

Merged
merged 1 commit into from Jan 13, 2022

Conversation

thymoze
Copy link
Contributor

@thymoze thymoze commented Dec 14, 2021

Description

As part of the update to ffmpeg 4.4 the implementation of avcodec_encode_audio2() was changed to a compatibility wrapper around avcodec_send_frame()/avcodec_receive_packet() (See ffmpeg source v4.4 and v4.3). This now seems to require the channel count to be set according to the channel layout.

A little more in-depth explanation of the problem:

Encoding failed due to the comparison right here as src (the frame we passed in) still has channels=0 while dst (a frame created with the exact same parameters as src) had it's channel count set according to the channel layout right here. Both functions are called from av_frame_ref().

I suspect that the same issue occurs in EncoderFFmpeg (the only other usage avcodec_encode_audio2()) as neither m_ResampledFrame or m_BufferFrame ever have channels set but I don't know enough about the codebase to verify the behaviour or test the fix.

Motivation and context

Fixes #20398

How has this been tested?

I tested AC3 transcoding on my NVIDIA Shield with various different files. I don't really know what else could be impacted but in any case it would probably be nice to get some people from #20398 to test on different platforms.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@popcornmix
Copy link
Member

LGTM.

@lrusak lrusak added Component: Audio Type: Fix non-breaking change which fixes an issue v20 Nexus labels Dec 14, 2021
@lrusak lrusak added this to the Nexus 20.0 Alpha 1 milestone Dec 14, 2021
Copy link
Contributor

@lrusak lrusak left a comment

Choose a reason for hiding this comment

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

Probably fine if it fixes the issue.

Thanks for the contribution!

@thymoze
Copy link
Contributor Author

thymoze commented Dec 14, 2021

Alright, just for completeness' sake I just tried ripping a cd (which seems to be the only place EncoderFFmpeg is used) and was able to verify the same problem. It's gonna take me a moment to setup a windows build to check that this will really fix it but then I'll add the change to EncoderFFmpeg as well.

@lrusak
Copy link
Contributor

lrusak commented Dec 14, 2021

Alright, just for completeness' sake I just tried ripping a cd (which seems to be the only place EncoderFFmpeg is used) and was able to verify the same problem. It's gonna take me a moment to setup a windows build to check that this will really fix it but then I'll add the change to EncoderFFmpeg as well.

Before digging too much maybe make a note of the changes here: #20429

@thymoze
Copy link
Contributor Author

thymoze commented Dec 14, 2021

Good call, then there's no need to change anything here. Even better 👍🏽

@hbbernardo
Copy link

hbbernardo commented Jan 13, 2022

Sorry for chiming in uninvited.

But what else is needed for this to get merged?

I'm only asking because I want to daily drive Libreelec with Nexus and this fix will make it possible to keep using as a media center.

Please, let us know.

Thanks

Libreelec forum thread: https://forum.libreelec.tv/thread/24741-librelec-nightly-dolby-digital-transcoding-bug/#post165351

@enen92
Copy link
Member

enen92 commented Jan 13, 2022

It has two approvals, just needs a green checkmark in Jenkins.

Jenkins build and merge

@hbbernardo
Copy link

It has two approvals, just needs a green checkmark in Jenkins.

Jenkins build and merge

Thanks for replying to it.

Forgive my ignorance. Jenkins is an automatic process name or a developer who still will revise it?

@jenkins4kodi jenkins4kodi merged commit e976cd0 into xbmc:master Jan 13, 2022
@hbbernardo
Copy link

hbbernardo commented Jan 22, 2022

Hi.

Although the fix was applied and it worked.

I'm encountering weird behavior.

After a transcode into DD Is done. As in when I exit the video being played. I noticed that I lost all the GUI sounds.

When I looked into my receiver it is 'stuck' into Dolby Digital. If I try to play another movie with DTS I got no sound.
Because my receiver is still stuck into Dolby Digital. The only way I got proper DTS is when I try a couple of times. Closing and reopening the same video with DTS.

I did a log where I hope this behavior was captured.

Here's the log: http://ix.io/3MHs

I've also posted this on Libreelec Forum. And I assure you. The problem is real. And it's only getting worse.

Please, let me know if the logs capture something.

https://forum.libreelec.tv/thread/24741-librelec-nightly-dolby-digital-transcoding-bug/?postID=165671#post165671

PS: I also own a Vero 4K (OSMC) and before this issue, there wasn't any difference between the two devices regarding this issues mentioned above.

The only difference is that Vero4K still is running Kodi 19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Audio Type: Fix non-breaking change which fixes an issue v20 Nexus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AC3 transcoding fails on Kodi 20
7 participants