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

Improve and fix issue in audioconv module #40

Merged
merged 3 commits into from
Aug 24, 2024

Conversation

mitsuki31
Copy link
Owner

@mitsuki31 mitsuki31 commented Aug 24, 2024

Overview

This pull request addresses several issues and enhancements within the audioconv module. The focus is on improving the reliability of the ffmpeg binary check, ensuring consistent error logging even when the quiet option is enabled, and enhancing the JSDoc documentation for better clarity and maintainability.

Changes Made

Fixes

Improvement to ffmpeg Binary Check and Error Handling

  • Removed the use of the promisified exec call to check the ffmpeg version, replacing it with the more reliable spawnSync approach, which prevents asynchronous issues and race conditions.
  • Refined the environment variable check for FFMPEG_PATH, adding checks to ensure it is neither null nor undefined before proceeding with the binary check.
  • Enhanced the splitOptions function to properly handle options that do not have associated values, ensuring that they are not accidentally concatenated with the next option in the list.
  • Streamlined the error handling process during the ffmpeg check, making the logging process clearer and more consistent.

Ensuring Error Logging Works with quiet Option Enabled

  • Refactored the writeErrorLog function to return a promise, improving the handling of errors during the log-writing process.
  • Updated the error handling within the convertAudio function to ensure that error logs are still written even when the quiet option is enabled, improving reliability in quiet modes.
  • Refined the logic for log and console error outputs, making them conditional based on the quiet option to provide better control over the reporting process.
  • Added more robust handling for the log stream, ensuring that the promise resolves on finish and properly rejects on error, enhancing the reliability of the logging process.

Documentation Updates

Enhancements to JSDoc Documentation and Type Definitions

  • Added default values to the inputOptions and outputOptions fields within the ConvertAudioOptions typedef, improving the clarity of the optional parameters and their usage.
  • Introduced a new FFmpegInfo typedef to document the progress data emitted by FFmpeg during the conversion process, aiding in better understanding and usage of the progress event.
  • Expanded the JSDoc comments for the writeErrorLog function, providing detailed documentation on the format of error logs, and specifying that the function now returns a Promise.
  • Updated the resolveOptions function’s JSDoc to include a module reference, improving the overall documentation structure and accessibility.
  • Enhanced the documentation for createConversionProgress, detailing the function’s parameters and return values, and ensuring that it is correctly documented for future use.
  • Marked the defaultOptions object as readonly in the documentation, clearly defining its role in the conversion process and preventing unintended modifications.

Fixed Issue

Example Issue with splitOptions Before Fix

Before the fix, when an option was provided without a corresponding value, it could get concatenated with the next option, leading to incorrect behavior:

const outputOptions = '-f -acodec libmp3lame -ar 44100';
splitOptions(options);

Expected output:

['-f', '-acodec libmp3lame', '-ar 44100']

Actual output before fix:

['-f -acodec', '-acodec libmp3lame', '-ar 44100']

In this case, -f and -acodec get concatenated, leading to an invalid option.


Example Corrected Behavior After Fix

After the fix, splitOptions correctly handles options without values, ensuring they remain separate:

const options = '-f -acodec libmp3lame -ar 44100';
splitOptions(options);

Correct output:

['-f', '-acodec libmp3lame', '-ar 44100']

Now, each option is treated as an individual entry, preserving the integrity of the options passed to FFmpeg.


Impact

  • These changes improve the robustness and maintainability of the audioconv module, addressing key issues that could lead to unreliable behavior in production environments.
  • The enhanced documentation provides a solid foundation for future development and usage of the module, making it easier for others to contribute and maintain.

- Removed the promisified `exec` call for `ffmpeg -version` check and replaced it with a more reliable `spawnSync` approach.
- Refined the environment variable check for `FFMPEG_PATH` to ensure it is not null or undefined before proceeding.
- Enhanced `splitOptions` to correctly handle options without values, avoiding accidental concatenation with subsequent options.
- Streamlined error handling and logging for better clarity during the `ffmpeg` check process.
- Refactored `writeErrorLog` to return a promise, improving error handling when writing to the log file.
- Updated the error handling within `convertAudio` to ensure that error logs are written even when the `quiet` option is enabled (set to `true`).
- Refined log and console error outputs to make them conditional on the `quiet` option, improving clarity and control over error reporting.
- Added better handling for the log stream, including resolving on `finish` and rejecting on `error` to ensure reliability in the logging process.
- Added default values to `inputOptions` and `outputOptions` in the `ConvertAudioOptions` typedef, improving clarity on optional parameters.
- Introduced `FFmpegInfo` typedef to document progress data emitted by FFmpeg during conversion.
- Added JSDoc comments for `writeErrorLog`, including format details for error logs, and ensured it returns a Promise.
- Updated `resolveOptions` to include its module reference in the JSDoc comment.
- Added detailed documentation for `createConversionProgress` and `splitOptions`, including parameter types and return values.
- Updated the `defaultOptions` object to be readonly, and clarified its role in the conversion process.
@mitsuki31 mitsuki31 added refactor Refactor and enhancement changes bugfix Fixes any issue and bug minor Minor changes labels Aug 24, 2024
@mitsuki31 mitsuki31 self-assigned this Aug 24, 2024
Copy link

codecov bot commented Aug 24, 2024

Codecov Report

Attention: Patch coverage is 16.66667% with 25 lines in your changes missing coverage. Please review.

Project coverage is 42.05%. Comparing base (3a66580) to head (91365f8).
Report is 4 commits behind head on master.

Files Patch % Lines
lib/audioconv.js 16.66% 25 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   42.75%   42.05%   -0.70%     
==========================================
  Files           7        7              
  Lines         407      409       +2     
==========================================
- Hits          174      172       -2     
- Misses        233      237       +4     
Flag Coverage Δ
Ubuntu 42.05% <16.66%> (-0.70%) ⬇️
Windows 42.05% <16.66%> (-0.70%) ⬇️
macOS 42.05% <16.66%> (-0.70%) ⬇️
unittests 42.05% <16.66%> (-0.70%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mitsuki31 mitsuki31 changed the title Improve and fixes issue in audioconv module Improve and fix issue in audioconv module Aug 24, 2024
@mitsuki31 mitsuki31 merged commit c2bc654 into master Aug 24, 2024
12 of 14 checks passed
@mitsuki31 mitsuki31 deleted the fix/audioconv-module-improvement-and-fixes branch August 24, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes any issue and bug minor Minor changes refactor Refactor and enhancement changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant