On 1/21/2016 12:11 PM, Hendrik Leppkes wrote: > On Thu, Jan 21, 2016 at 1:50 PM, wm4 <nfx...@googlemail.com> wrote: >> On Thu, 21 Jan 2016 11:46:03 +0100 >> Hendrik Leppkes <h.lepp...@gmail.com> wrote: >> >>> On Fri, Jan 15, 2016 at 6:12 AM, James Almer <jamr...@gmail.com> wrote: >>>> On 1/14/2016 9:59 PM, Andreas Cadhalpun wrote: >>>>> On 14.01.2016 17:25, foo86 wrote: >>>>>> Full diff output has been omitted for deleted files. If git complains >>>>>> about >>>>>> applying the first patch, this can be also pulled from dca-replace >>>>>> branch at >>>>>> [1]. >>>>> >>>>> I'd prefer if you would post full patches here, i.e. including deleted >>>>> files. >>>> >>>> For future, smaller patches if anything. Doing so here would mean >>>> thousands of >>>> extra lines for no reason or benefit. And i remember there being a problem >>>> with >>>> git send-email regarding huge emails. >>>> >>>>> >>>>> That aside, the new decoder seems fine from a security point of view, with >>>>> only some rare overflows in the dsp functions left. >>>>> >>>>> However, this series breaks FATE. >>>>> The fate-dca-xll test should probably be disabled, as the reference was >>>>> created >>>>> with the old, not bitexact decoder. (It currently fails due to the >>>>> disable_xll >>>>> option being gone, but fixing that reveals the changed output.) >>>>> >>>>> The checkasm test needs to be updated: >>>> >>>> Removed, actually. The new lfe_fir dsp functions don't yet have arch >>>> optimized >>>> versions, so even if you adapt this code now it will not be really tested >>>> and >>>> reported as part of the checkasm output since there's nothing to compare >>>> the C >>>> version with. >>>> It can be readded and adapted once said arch optimized functions are >>>> written. >>>> >>> >>> Other than the comments already made here, this set looks fine to me. >>> The missing changes are trivial removals of (now broken) tests, so I >>> could do that when pushing as well if we all agree. >>> >>> New tests should then be added afterwards. >>> >>> Everyone else, if you still have outstanding comments on this set, >>> please do speak up soon, otherwise I would say we give it another >>> couple days and then just push it, so we can move forward. >> >>> Anyone any opinions if this should be squashed to avoid leaving us >>> without a dca decoder for a few commits? Probably should be, right? >> >> Why would this be important? Except maybe that it would be nice if >> FATE worked at any point in the history (to make bisecting easier). > > That would mean removing and re-adding some stuff, which seems > somewhat annoying. But I don't really care either way.
We can either apply patches 2 to 6 first then patches 1 and 7 squashed into one, or all 7 as they are. Assuming the latter, all dca tests should be commented out and the lfe check in checkasm removed with patch 1, then the dca tests re-enabled (and dca-xll updated) with patch 7. With the former, it would be a matter of updating dca-xll and checkasm in the squashed patch. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
