Attempt to fix FFMPEG 5.0 compatibility #5644

bjuncek · 2022-03-18T14:11:37Z

Attempting to address #5616 via simple API update.
Should be properly tested in FB internals.

facebook-github-bot · 2022-03-18T14:11:45Z

💊 CI failures summary and remediations

As of commit 3c7e998 (more details on the Dr. CI page):

1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job	Step	Action
^{cmake_linux_gpu}	^{Build torchvision C++ distribution and test}	🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

datumbox

@bjuncek Thanks for fixing this. The changes look good to me. I've only added one comment for your attention.

Our CI is currently broken, so I propose not to merge this until #5648 is merged and it's properly tested on FB infra as you suggested. We'll get back to you on the latter.

torchvision/csrc/io/decoder/subtitle_stream.cpp

jdsgomes

I have imported the PR and pointed out a few things to be considered.

jdsgomes · 2022-03-21T14:45:14Z

torchvision/csrc/io/decoder/decoder.cpp

+  // update 03/22: moving memory management to ffmpeg
+  AVPacket* avPacket;
+  avPacket = av_packet_alloc();
+  if (avPacket == NULL) {


can this NULL be replaced by nullptr? the internal linter CLANGTID is complaining about this line

Should we return inside this if-clause, otherwise we will access the NULL pointer in the next line.

torchvision/csrc/io/decoder/decoder.cpp

jdsgomes · 2022-03-22T16:16:31Z

@bjuncek I made a few changes in an attempt to fix the memory leak and the static code analysis warnings, but there are a few things that came up that would need discussion when you are back as you will probably have more context:

if you agree with the approach here and here, we probably need to follow the same in torchvision/csrc/io/decoder/subtitle_stream.cpp
it is not 100% clear to me what is the difference between av_packet_unref and av_packet_free. From what I can gather in the documentation, the former unreferences the inner buffer and resets the default values and the later frees the actual avPacket object but please confirm that is the case and that we need both in the way it has been used

bjuncek · 2022-03-23T12:01:52Z

if you agree with the approach here and here, we probably need to follow the same in torchvision/csrc/io/decoder/subtitle_stream.cpp

Yup, have added the changes; this is something you might need checking in FB infra as subtitle stream was (yet) not used in TV, but might have been used internally (I've been long gone).

it is not 100% clear to me what is the difference between av_packet_unref and av_packet_free. From what I can gather in the documentation, the former unreferences the inner buffer and resets the default values and the later frees the actual avPacket object but please confirm that is the case and that we need both in the way it has been used

My understanding in this case is the same as yours, and I do thing we'd need both; do these changes pass the static memory leak checks?

torchvision/csrc/io/decoder/decoder.cpp

torchvision/csrc/io/decoder/subtitle_stream.cpp

torchvision/csrc/io/decoder/decoder.cpp

datumbox

Thanks for your work @bjuncek. I think it looks good.

@jdsgomes Could you check if this works OK internally?

jdsgomes · 2022-03-24T14:50:07Z

torchvision/csrc/io/decoder/subtitle_stream.cpp

+    LOG(ERROR)
+        << "decoder as not able to allocate the subtitle-specific packet.";
+    // alternative to ENOMEM
+    return AVERROR_BUFFER_TOO_SMALL;


Should we align the error code with the corresponding bit in decoder.cpp

@jdsgomes The two methods return different enums. Originally Bruno had them synced; no strong opinions on my side.

@jdsgomes would you like me to change it to match?

I would prefer for consistency, unless there is a reason not to. Other than that the PR looks good and the internal tests are passing. Thank you for these changes

jdsgomes · 2022-03-24T14:58:34Z

torchvision/csrc/io/decoder/subtitle_stream.cpp

@@ -43,21 +43,34 @@ int SubtitleStream::initFormat() {
 int SubtitleStream::analyzePacket(const AVPacket* packet, bool* gotFrame) {
  // clean-up
  releaseSubtitle();
+
+  // FIXME: should this even be created?


agree - I am also not sure if this should be created. Is the plan to follow up on this?

Yes, but I think this would require a bit of coordination on Meta's side, as this is not used nor tested in TV.

jdsgomes · 2022-03-25T09:55:41Z

For reference the internal tests are passing: https://www.internalfb.com/diff/D35115411

bjuncek · 2022-03-25T10:10:31Z

@datumbox I'll leave it up to you to decide if it's ready for merge.

jdsgomes · 2022-03-25T11:24:19Z

I have a nit regarding he return codes but definitely non blocking.

datumbox

@bjuncek LGTM, thanks!

@jdsgomes I agree that a good API should align the return error codes across functions. I'm OK if you want to merge this now and follow up in a new PR. I'm also OK if you want to push the change directly on this branch. I leave it to you how we should proceed. At any case, we should merge this soon. We are already a week with broken CI and the issue is masking other potential problems that are not reported to the release engineering team.

torchvision/csrc/io/decoder/decoder.cpp

Summary: * fix for FFMPEG 5.0 * Attempt to fix memory leak * early stop when mem alloc fails * fix c lint error * fix bug * revert changes * remove incorrect av_packet_free * add `av_packet_free` to subtitle stream * Addressing subtitle stream comments * addressing decoder changes * nit * addressing datumbox concernsz * averror push * addressing comments * Apply suggestions from code review * add new error code to documentation * re-introducing timeout * fix c++ syntax (Note: this ignores all push blocking failures!) Reviewed By: datumbox Differential Revision: D35216769 fbshipit-source-id: e3489471406663c8e71fc239164a682993771db3 Co-authored-by: Bruno Korbar <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]> Co-authored-by: Joao Gomes <[email protected]> Co-authored-by: Joao Gomes <[email protected]>

* fix for FFMPEG 5.0 * Attempt to fix memory leak * early stop when mem alloc fails * fix c lint error * fix bug * revert changes * remove incorrect av_packet_free * add `av_packet_free` to subtitle stream * Addressing subtitle stream comments * addressing decoder changes * nit * addressing datumbox concernsz * averror push * addressing comments * Apply suggestions from code review * add new error code to documentation * re-introducing timeout * fix c++ syntax Co-authored-by: Bruno Korbar <[email protected]> Co-authored-by: Vasilis Vryniotis <[email protected]> Co-authored-by: Joao Gomes <[email protected]> Co-authored-by: Joao Gomes <[email protected]> (cherry picked from commit da03f51)

fix for FFMPEG 5.0

2a05e5f

pytorch-bot bot added the ciflow/default label Mar 18, 2022

facebook-github-bot added the cla signed label Mar 18, 2022

Merge branch 'main' into bkorbar/5616_v2

43922a6

bjuncek added module: io module: video labels Mar 18, 2022

datumbox mentioned this pull request Mar 21, 2022

Prefer nvidia channel for conda builds #5648

Merged

Merge branch 'main' into bkorbar/5616_v2

866b30a

datumbox reviewed Mar 21, 2022

View reviewed changes

torchvision/csrc/io/decoder/subtitle_stream.cpp Show resolved Hide resolved

jdsgomes reviewed Mar 21, 2022

View reviewed changes

datumbox and others added 8 commits March 21, 2022 18:55

Merge branch 'main' into bkorbar/5616_v2

d0cadf9

Attempt to fix memory leak

3a927c6

early stop when mem alloc fails

7984483

fix c lint error

f9c8987

Merge branch 'main' into bkorbar/5616_v2

5e11472

fix bug

4c6c8fc

revert changes

08232e4

remove incorrect av_packet_free

2f79d6c

datumbox mentioned this pull request Mar 22, 2022

Port Multi-weight support from prototype to main TorchVision #5618

Merged

6 tasks

add av_packet_free to subtitle stream

d124ee1

datumbox reviewed Mar 23, 2022

View reviewed changes

torchvision/csrc/io/decoder/decoder.cpp Outdated Show resolved Hide resolved

torchvision/csrc/io/decoder/subtitle_stream.cpp Outdated Show resolved Hide resolved

bjuncek and others added 4 commits March 24, 2022 10:06

Merge branch 'main' into bkorbar/5616_v2

df9cb7e

Addressing subtitle stream comments

2f7c02a

addressing decoder changes

926a11e

nit

2602ef4

datumbox reviewed Mar 24, 2022

View reviewed changes

torchvision/csrc/io/decoder/subtitle_stream.cpp Outdated Show resolved Hide resolved

datumbox reviewed Mar 24, 2022

View reviewed changes

torchvision/csrc/io/decoder/subtitle_stream.cpp Show resolved Hide resolved

Bruno Korbar added 2 commits March 24, 2022 10:59

addressing datumbox concernsz

b978ba1

averror push

0f02ed9

datumbox reviewed Mar 24, 2022

View reviewed changes

torchvision/csrc/io/decoder/subtitle_stream.cpp Show resolved Hide resolved

torchvision/csrc/io/decoder/subtitle_stream.cpp Show resolved Hide resolved

torchvision/csrc/io/decoder/decoder.cpp Show resolved Hide resolved

torchvision/csrc/io/decoder/decoder.cpp Outdated Show resolved Hide resolved

addressing comments

50d234f

datumbox reviewed Mar 24, 2022

View reviewed changes

jdsgomes reviewed Mar 24, 2022

View reviewed changes

Merge branch 'main' into bkorbar/5616_v2

1ffc0ad

jdsgomes approved these changes Mar 25, 2022

View reviewed changes

datumbox approved these changes Mar 25, 2022

View reviewed changes

datumbox added the enhancement label Mar 25, 2022

jdsgomes reviewed Mar 25, 2022

View reviewed changes

torchvision/csrc/io/decoder/decoder.cpp Outdated Show resolved Hide resolved

jdsgomes reviewed Mar 25, 2022

View reviewed changes

torchvision/csrc/io/decoder/decoder.cpp Outdated Show resolved Hide resolved

jdsgomes and others added 6 commits March 25, 2022 12:52

Apply suggestions from code review

f0212c0

Merge branch 'main' into bkorbar/5616_v2

41c0ad9

add new error code to documentation

1811fa1

re-introducing timeout

93e0863

fix c++ syntax

3487c68

Merge branch 'main' into bkorbar/5616_v2

3c7e998

jdsgomes merged commit da03f51 into pytorch:main Mar 25, 2022

This was referenced Apr 1, 2022

Torchvision doesn't compile against ffmpeg-5 #5616

Closed

The dependency ffmpeg 4.3 has critical CVEs #4191

Closed

bjuncek deleted the bkorbar/5616_v2 branch April 14, 2022 10:39

pmeier mentioned this pull request May 2, 2022

A problem related to FFmpeg when building from source. #5928

Closed

hmaarrfk mentioned this pull request Jul 19, 2022

Rebuild for pytorch112 conda-forge/torchvision-feedstock#58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to fix FFMPEG 5.0 compatibility #5644

Attempt to fix FFMPEG 5.0 compatibility #5644

bjuncek commented Mar 18, 2022

facebook-github-bot commented Mar 18, 2022 •

edited

Loading

datumbox left a comment

jdsgomes left a comment

jdsgomes Mar 21, 2022

jdsgomes Mar 21, 2022

jdsgomes commented Mar 22, 2022 •

edited

Loading

bjuncek commented Mar 23, 2022

datumbox left a comment

jdsgomes Mar 24, 2022

datumbox Mar 24, 2022

bjuncek Mar 24, 2022

jdsgomes Mar 25, 2022

jdsgomes Mar 24, 2022

bjuncek Mar 24, 2022

jdsgomes commented Mar 25, 2022

bjuncek commented Mar 25, 2022

jdsgomes commented Mar 25, 2022

datumbox left a comment •

edited

Loading

Attempt to fix FFMPEG 5.0 compatibility #5644

Attempt to fix FFMPEG 5.0 compatibility #5644

Conversation

bjuncek commented Mar 18, 2022

facebook-github-bot commented Mar 18, 2022 • edited Loading

💊 CI failures summary and remediations

1 failure not recognized by patterns:

datumbox left a comment

Choose a reason for hiding this comment

jdsgomes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsgomes commented Mar 22, 2022 • edited Loading

bjuncek commented Mar 23, 2022

datumbox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsgomes commented Mar 25, 2022

bjuncek commented Mar 25, 2022

jdsgomes commented Mar 25, 2022

datumbox left a comment • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Mar 18, 2022 •

edited

Loading

jdsgomes commented Mar 22, 2022 •

edited

Loading

datumbox left a comment •

edited

Loading