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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
