[FFmpeg-devel] [PATCH] Revert "Merge commit '741b494fa8cd28a7d096349bac183893c236e3f9'"

[Hendrik Leppkes]

On Sat, Dec 19, 2015 at 10:35 AM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Thu, Dec 17, 2015 at 02:46:40PM +0100, Hendrik Leppkes wrote:
>> On Thu, Dec 17, 2015 at 2:06 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > From: Michael Niedermayer <michael at niedermayer.cc>
>> >
>> > This fixes a regression of the sample from Ticket 2371
>> >
>> > This reverts commit bc66451e5e903698ee0500faf04c1214f3dd157f, reversing
>> > changes made to 9d1fb9ef313e0fb709ac4c35c7bf00264963fd85.
>> > ---
>> >  libavcodec/h264.h       |    8 +++++++
>> >  libavcodec/h264_refs.c  |   57 ++++++++++++++++++++++++++---------------------
>> >  libavcodec/h264_slice.c |   17 +++++++++++++-
>> >  3 files changed, 56 insertions(+), 26 deletions(-)
>>
>> Reverting big commits to fix one sample and making future merges
>> harder or impossible seems not like the best option, unless it can be
>> shown that the entire change is flawed, and not just a small bug.
>> Otherwise, it should be attempted to fix regressions the usual way -
>> by fixing things, not reverting.
>>
>> Once reverted, the change will never come back, we all know that.
>>
>> At least thats IMHO. You are the maintainer of the H264 decoder, so if
>> you want to revert it, feel free.
>
> Calculatig the default ref seems to take between 800 and 4700 or so
> cpu cycles (messured with fate & START_TIMER)
>
> tests/data/fate/filter-paletteuse-sierra2_4a.err:  46958 decicycles in h264_initialise_ref_list,      64 runs,      0 skips
> tests/data/fate/h264-conformance-caba3_sony_c.err:  26963 decicycles in h264_initialise_ref_list,     256 runs,      0 skips
> tests/data/fate/h264-conformance-frext-hcafr4_hhi_a.err:  29306 decicycles in h264_initialise_ref_list,      15 runs,      1 skips
> tests/data/fate/h264-conformance-sva_cl1_e.err:   7956 decicycles in h264_initialise_ref_list,     128 runs,      0 skips
> ...
>
> with the default lists the time spend in Calculating them after the
> first slice and including the checks needed to find out if they need
> to be recalculated seem to be always below 500 cycles in fate
>
> tests/data/fate/h264-conformance-cvnlfi1_sony_c.err:   4196 decicycles in CMP,     256 runs,      0 skips
> tests/data/fate/h264-conformance-cvfi1_sony_d.err:   3976 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-cvfi1_sony_d.err:   4142 decicycles in CMP,     256 runs,      0 skips
> tests/data/fate/h264-conformance-basqp1_sony_c.err:    381 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-cvnlfi2_sony_h.err:   3757 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-cvnlfi2_sony_h.err:   3687 decicycles in CMP,     256 runs,      0 skips
> tests/data/fate/h264-conformance-ci1_ft_b.err:   4588 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-ci1_ft_b.err:   4754 decicycles in CMP,     256 runs,      0 skips
> tests/data/fate/h264-conformance-cvfc1_sony_c.err:   4146 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-cabaci3_sony_b.err:   2739 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-cabaci3_sony_b.err:   2732 decicycles in CMP,     256 runs,      0 skips
> tests/data/fate/h264-conformance-cabaci3_sony_b.err:   2878 decicycles in CMP,     512 runs,      0 skips
> tests/data/fate/h264-conformance-ba1_ft_c.err:   4266 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-ba1_ft_c.err:   4358 decicycles in CMP,     256 runs,      0 skips
> tests/data/fate/h264-conformance-cvfi2_sony_h.err:   4347 decicycles in CMP,     128 runs,      0 skips
> tests/data/fate/h264-conformance-cvfi2_sony_h.err:   4179 decicycles in CMP,     256 runs,      0 skips
>
> its possible the extra list causes harder to meassure slowdowns
> elsewhere of course (cache hit/miss ratio, more fields in the context,
> whatever)
>
> but for files with many small slices recalculating the same list
> for each seems a waste of time to me.
>
> i also suspect that libav had a bug in their default ref list
> code or at least their checks look less complete than ours prior
> to the removial in both trees
>
> I think the decission about which way to go here should be yours
> as you might (or might not) be affected by this causing extra work
> in the future
>

Libavs patch doesn't actually fix the sample, and considering there
may be a slowdown, revert is fine with me. Its not like H264 is going
to change substantially in the future.

- Hendrik