Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

FFMPEG In-built Support #161

Closed
wants to merge 9 commits into from
Closed

FFMPEG In-built Support #161

wants to merge 9 commits into from

Conversation

iim-ayush
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:

Added in-built support for FFMPEG without disturbing createAudioResource function.

Created a whole new function in audio - resource file. [ createFFMPEGResource() ]

Status and versioning classification:

Resolves #143 , #117 , #141 , #124.

@codecov
Copy link

codecov bot commented Jul 26, 2021 •
edited
Loading

Codecov Report

Merging #161 (3ced618) into main (f1869a9) will decrease coverage by 0.16%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
- Coverage   73.84%   73.67%   -0.17%     
==========================================
  Files          21       21              
  Lines         906      927      +21     
  Branches      221      229       +8     
==========================================
+ Hits          669      683      +14     
- Misses        235      242       +7     
  Partials        2        2
Impacted Files Coverage Δ
src/audio/AudioResource.ts 88.88% <66.66%> (-7.78%) ⬇️

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 f1869a9...3ced618. Read the comment docs.

amishshah
amishshah suggested changes Aug 7, 2021
View reviewed changes
Copy link
Member

@amishshah amishshah left a comment

Choose a reason for hiding this comment

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

I think I'm generally happy with this PR - I like how it doesn't interfere with existing code and instead builds on top as a utility.

However, two thoughts:

  1. It seemingly only support Ogg opus output from FFmpeg (not all FFmpeg instances will support this, or for users that want inline volume, this is unnecessary)
  2. No tests - if you could add tests that would be good, otherwise if you really do not want to I can add them once the PR is ready, but either way I think we should have tests here before merging.

Would appreciate if you could figure out a nice way of deciding whether to use Opus or PCM output here, maybe using the existing logic to help decide this

@stijnb1234 stijnb1234 mentioned this pull request Aug 8, 2021
Closed
iim-ayush
iim-ayush commented Aug 8, 2021
View reviewed changes
Copy link
Contributor Author

@iim-ayush iim-ayush left a comment

Choose a reason for hiding this comment

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

I have done everything. It would be great if you make test files and check it's actual use.

twlite
twlite reviewed Aug 9, 2021
View reviewed changes
Copy link

@twlite twlite left a comment

Choose a reason for hiding this comment

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

🤔 imo options like reconnect, reconnect_time, seek should be handled by the users since this api provides arguments option? Users should have full control over this 😕

@iim-ayush
Copy link
Contributor Author

🤔 imo options like reconnect, reconnect_time, seek should be handled by the users since this api provides arguments option? Users should have full control over this 😕

I know about this.

Options like reconnect, reconnect_time, seek are for those users who particularly don't know much about setting up FFMPEG args.

It will be easier for people who have just shifted from discord voice old module to find seek option. Moreover, FFMPEG has some issues with connection drops. So I added reconnect args only to help to fix that issue.

arguments allow users to have full control what they need to pass and what not to pass. [ without options like reconnect, reconnect_time, seek ]

iim-ayush
iim-ayush commented Aug 10, 2021
View reviewed changes
Copy link
Contributor Author

@iim-ayush iim-ayush left a comment

Choose a reason for hiding this comment

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

Tests have been added. I have included only 2 tests since FFMPEG either convert it to OPUS or PCM according to inLineVolume.

@iim-ayush iim-ayush marked this pull request as draft August 19, 2021 11:03
@iim-ayush
Copy link
Contributor Author

Thanks, I got my reply for re-review.

@iim-ayush iim-ayush closed this Aug 19, 2021
@iim-ayush iim-ayush reopened this Aug 25, 2021
@iim-ayush
Copy link
Contributor Author

Let's see how much time will it take to merge

@iim-ayush iim-ayush marked this pull request as ready for review August 25, 2021 02:50
@amishshah amishshah closed this Aug 25, 2021
@LingleDev
Copy link

can you merge this? it would be very helpful

@iim-ayush
Copy link
Contributor Author

can you merge this? it would be very helpful

I am going to make another pull request soon. So wait till then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@twlite twlite twlite left review comments

@amishshah amishshah Awaiting requested review from amishshah

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FFmpeg custom arguments
4 participants
@iim-ayush @LingleDev @amishshah @twlite