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

[Michael Niedermayer]

On Sat, Jan 02, 2016 at 06:20:53PM +0100, Hendrik Leppkes wrote:
> 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.

ive posted an alternative patch if people prefer not to reintroduce
the default list or rather it reintroduces only a 2 element list
to fix the regression

ill push whatever solution people prefer

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160103/ed003600/attachment.sig>