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: Create, use av_err2string() #689

Merged
merged 3 commits into from Aug 11, 2021
Merged

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jun 11, 2021

(Note: Split out of #661)

My commit message for this reads:

  • Previously 'av_make_error_string' was defined in FFmpegUtilities.h
    for the sole purpose of redefining av_err2str() as a call to that
    function. av_err2str() was then used in our code, often in string
    contexts where its output was cast to std::string.
  • Since that was excessively circular, instead the function is named
    av_err2string(), and it's used directly in contexts where a
    std::string is expected.
  • av_err2str() is still #defined as av_err2string(...).c_str()

What actually happened was worse than I made it sound:

  1. av_make_error_string() wrapped FFmpeg's av_strerror() to return a std::string, for C++ compatibility
  2. av_err2str() was then redefined:
    #define av_err2str(errnum) av_make_error_string(errnum).c_str()
    thereby producing the char* version of av_make_error_string()
  3. Most All uses of av_err2str(errnum) in the code were then written as
    (std::string)av_err2str(errnum)

Like I said, dizzyingly circular. So all of those (std::string)av_err2str(errnum)s are now replaced with av_err2string(errnum). No casting needed.

- Previously 'av_make_error_string' was defined in FFmpegUtilities.h
  for the sole purpose of redefining `av_err2str()` as a call to that
  function. `av_err2str()` was then used in our code, often in string
  contexts where its output was cast to `std::string`.
- Since that was excessively circular, instead the function is named
  `av_err2string()`, and it's used directly in contexts where a
  std::string is expected.
- `av_err2str()` is still #defined as `av_err2string(...).c_str()`
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #689 (41f7eaa) into develop (dd85900) will decrease coverage by 0.00%.
The diff coverage is 21.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #689      +/-   ##
===========================================
- Coverage    50.40%   50.39%   -0.01%     
===========================================
  Files          155      155              
  Lines        13291    13292       +1     
===========================================
  Hits          6699     6699              
- Misses        6592     6593       +1     
Impacted Files Coverage Δ
src/FrameMapper.cpp 91.91% <0.00%> (-0.23%) ⬇️
src/FFmpegWriter.cpp 59.70% <10.00%> (ø)
src/FFmpegUtilities.h 100.00% <100.00%> (ø)

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 dd85900...41f7eaa. Read the comment docs.

@github-actions github-actions bot added the conflicts A PR with unresolved merge conflicts label Jun 26, 2021
@github-actions
Copy link

Merge conflicts have been detected on this PR, please resolve.

@ferdnyc ferdnyc added ffmpeg Issues or PRs involving the a/v processing code and removed conflicts A PR with unresolved merge conflicts labels Jun 26, 2021
@github-actions
Copy link

github-actions bot commented Jul 3, 2021

Merge conflicts have been detected on this PR, please resolve.

@github-actions github-actions bot added the conflicts A PR with unresolved merge conflicts label Jul 3, 2021
@github-actions github-actions bot removed the conflicts A PR with unresolved merge conflicts label Jul 7, 2021
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Aug 11, 2021

I have no idea why I haven't merged this yet, so... doing so.

@ferdnyc ferdnyc merged commit fbe0242 into OpenShot:develop Aug 11, 2021
@ferdnyc ferdnyc deleted the av_err2string branch August 11, 2021 07:58
ferdnyc added a commit to ferdnyc/libopenshot that referenced this pull request Sep 27, 2021
- Previously 'av_make_error_string' was defined in FFmpegUtilities.h
  for the sole purpose of redefining `av_err2str()` as a call to that
  function. `av_err2str()` was then used in our code, often in string
  contexts where its output was cast to `std::string`.
- Since that was excessively circular, instead the function is named
  `av_err2string()`, and it's used directly in contexts where a
  std::string is expected.
- `av_err2str()` is still #defined as `av_err2string(...).c_str()`
ferdnyc added a commit to ferdnyc/libopenshot that referenced this pull request Sep 27, 2021
- Previously 'av_make_error_string' was defined in FFmpegUtilities.h
  for the sole purpose of redefining `av_err2str()` as a call to that
  function. `av_err2str()` was then used in our code, often in string
  contexts where its output was cast to `std::string`.
- Since that was excessively circular, instead the function is named
  `av_err2string()`, and it's used directly in contexts where a
  std::string is expected.
- `av_err2str()` is still #defined as `av_err2string(...).c_str()`
ferdnyc added a commit to ferdnyc/libopenshot that referenced this pull request Oct 27, 2021
- Previously 'av_make_error_string' was defined in FFmpegUtilities.h
  for the sole purpose of redefining `av_err2str()` as a call to that
  function. `av_err2str()` was then used in our code, often in string
  contexts where its output was cast to `std::string`.
- Since that was excessively circular, instead the function is named
  `av_err2string()`, and it's used directly in contexts where a
  std::string is expected.
- `av_err2str()` is still #defined as `av_err2string(...).c_str()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ffmpeg Issues or PRs involving the a/v processing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant