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

Fix Segfault if SAF is used with concat demuxer #846

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

Vorticity-Flux
Copy link

Description

If Storage Access Framework protocol is used with concat demuxer (https://trac.ffmpeg.org/wiki/Concatenate#demuxer) - ffmpeg-kit segfaults as saf_open and saf_close are called from a separate ffmpeg thread that is not attached with JNI and there are no checks if GetEnv was successful.

This PR adds code to attach/detach thread in saf_open and saf_close that fixes said segfault. After this change concat demuxer works with SAF with no further issues.

Type of Change

  • Bug fix

Checks

  • Changes support all (relevant) platforms (only Android is relevant for SAF)
  • Breaks existing functionality
  • Implementation is completed, not half-done
  • Is there another PR already created for this feature/bug fix

Tests (in Android code)

  1. Create SAF links from Android content URIs.
  2. Create concat demuxer input file (mylist.txt) with said SAF links (even one saf link here is enough to cause segfault):
file saf:1.mp4
file saf:2.mp4
  1. Call ffmpeg concat demuxer like so:
    ffmpeg -y -f concat -safe 0 -protocol_whitelist saf,file,crypto -i mylist.txt -c copy saf:3.mp4
    Segfault will occur (if fix was not applied).

@tanersener
Copy link
Collaborator

Thanks for submitting this PR.

I reviewed the changes. It looks like they will resolve the issue. However, I'm not sure this is the best way to fix this problem. This implementation attaches/detaches the same thread multiple times.

Also, PRs must be opened against the development branch, as we explained in the CONTRIBUTING.md guide.

@Vorticity-Flux Vorticity-Flux changed the base branch from main to development September 28, 2023 02:53
@Vorticity-Flux
Copy link
Author

Vorticity-Flux commented Sep 28, 2023

I agree that calling Attach/Detach for each JNI call is sub-optimal and should be avoided. I've only done it this way as the most simple change that addresses the problem. Performance impact of this implementation should be small as we are only calling attach/detach twice per SAF input file (assuming the number of input files is reasonable).

As an alternative to this PR the only implementation I can think of is to attach on the first call to saf_open/saf_close and detach in the end of input_thread if previously attached. However this would require changing input_thread in fftools_ffmpeg_demux.c (which is forked from ffmpeg) thus increasing difference with upstream and complicating upgrades. I personally prefer lower upkeep and better compatibility in this case as I perceive the performance impact of extra Attach/Detach cycle as minimal compared to actual ffmpeg workload. If this would be an implementation you prefer, I can submit a PR for it as well.

Attaching/detaching in saf_open/saf_close provides better compatibility, I am not sure that there are no other threads besides input_thread that ffmpeg can use for accessing the input files (concat demuxer may be one of many edge cases).

@tanersener
Copy link
Collaborator

Unfortunately, there is no perfect solution for this case. The alternative, attaching threads when they're created, will attach threads for cases which don't need attach/detach. And, ffmpeg upgrades will be more painful as you mentioned.

There are a few things I want to check before merging this PR. Let me test them. If they don't give us a better alternative then I'll merge this one.

@tanersener tanersener self-requested a review October 3, 2023 21:51
@tanersener
Copy link
Collaborator

Looks good to me.

@tanersener tanersener merged commit f0d4754 into arthenica:development Oct 3, 2023
13 checks passed
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.

None yet

2 participants