[FFmpeg-devel] [PATCH 0/6] h264_redundant_pps: Make bsf refcount-compliant

[Andreas Rheinhardt]

Commit c6a63e11092c975b89d824f08682fe31948d3686 made the parameter sets
of both the CodedBitstreamH264Context and CodedBitstreamH265Context
reference-counted and also stopped copying the parameter sets read from
input; instead, the content of the parameter set units read from input
are now shared with the parameter sets in the respective contexts. This
means that said parameter sets are usually not writeable and ignoring
this can have adverse consequences.

An example of this is seen in the h264_redundant_pps bitstream filter.
It modifies two parameters of the H264RawPPS returned as content of
parameter sets unit encountered in input. By modifying these two
parameters (or actually, by modifying weighted_pred_flag), the PPS used
to parse future input (i.e. from the next access unit onwards) has been
changed and the result are parsing errors, because now
pred_weight_table() is presumed, even when it actually isn't there.

(The usage in h264_metadata is not dangerous, since no parameters that
are essential for the parsing process are changed in said bitstream
filter. But this is of course against the principle of refcounted
data.)

Here is a sample for which this bitstream filter was designed:
https://www.dropbox.com/s/76l8kml3qleyovw/H264_redundant_pps.sample.mkv?dl=0
Using the current version of h264_redundant_pps leads to a flood of
error messages and reduces the size from 38.3 MB to 16.8 MB because of
discarded packets.

In order to fix this, I created a function to perform deep copies of
parameter sets and also functions to make the references to (content)
parameter sets writeable. This also fixes dangling pointers problems
(currently a shallow copy is performed when a parameter set is replaced
by a parameter set that is not refcounted; now it's a deep copy).

I am unsure what arguments the make_writeable function(s) should take: A
CodedBitstreamUnit * or simply an AVBuffer ** like the normal
av_buffer_make_writable. In the end I opted for the latter, because of
the similarity to the normal writeable-function and because I can't rule
out that there might be situations in which one wants to copy a
refcounted parameter set that is not part of a unit. But notice that
using the unit would have its advantages, too: One could automatically
update the content-buffer and one could use the CodedBitstreamUnitType
to know which type of parameter set it is and therefore reduce the
number of functions needed (given that the nal_unit_types of H.264 and
H.265 parameter sets don't overlap, one could even use one function for
all). If you want to, I'll change this.

I have also included a patch to create several of the free functions via
a macro. Their similarity allows this.

Furthermore, there are two more things wrong with h264_redundant_pps: It
heavily leaks memory when there are errors (and now there are always
errors). And using -loglevel verbose or above easily produces crashes,
because the wrong logging context has been used in an av_log. There are
patches to address these issues.

Finally, this patchset is (re)based on top of the current Git head, as
required. My earlier patchset "Improve performance of writing slices"
also modifies libavcodec/cbs_h2645.c, but at different places, so there
won't be merge conflicts and it will be easy to rebase one on top of
another.


Andreas Rheinhardt (6):
  cbs_h2645: Unify the free-functions via macro
  cbs_h2645: Do a deep copy for parameter sets
  cbs_h2645: Create functions to make parameter sets writeable
  h264_redundant_pps: Fix memleak in case of errors
  h264_redundant_pps: Make it reference-compatible
  h264_redundant_pps: Fix logging context

 libavcodec/cbs_h264.h               |   8 ++
 libavcodec/cbs_h2645.c              | 130 +++++++++++++++++-----------
 libavcodec/cbs_h265.h               |   9 ++
 libavcodec/h264_redundant_pps_bsf.c |  59 +++++++++----
 4 files changed, 138 insertions(+), 68 deletions(-)

-- 
2.19.0