| Message ID | 20180228221633.GW23018@michaelspb |
|---|---|
| State | Not Applicable |
| Headers | show |
On 3/1/18, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Feb 28, 2018 at 10:14:15PM +0200, Ivan Kalvachev wrote: >> Replace two bit handling loops and internal conditional branch >> with simple formula using few logical operations. >> >> The old function would generate wrong output >> if the input does not fit into 15 bits. >> >> Fix this by using 64 bit math and put_bits64(). >> This case should be quite rare, since the bug >> has not asserted itself. >> >> --- >> It's attempt for speed optimization, but in the >> process it turned out it needs also bugfixing. >> >> I only tested the old case of the code, >> to confirm i've implemented the correct function. >> >> Haven't done any benchmarks or run fate. > > fate fails: > Well, the good news is that it compiles and doesn't crash. The change of generated files is expected. The old code would write up to 63 bits with regular put_bits() that is limited to 31 bits. Are av_assert2() enabled during fate runs? (That's how put_bits() checks for bit length.) I'd like to know if my code is correct/incorrect before attempting to changing fate checksums. Best Regards
On Thu, Mar 01, 2018 at 03:52:10AM +0200, Ivan Kalvachev wrote: > On 3/1/18, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Feb 28, 2018 at 10:14:15PM +0200, Ivan Kalvachev wrote: > >> Replace two bit handling loops and internal conditional branch > >> with simple formula using few logical operations. > >> > >> The old function would generate wrong output > >> if the input does not fit into 15 bits. > >> > >> Fix this by using 64 bit math and put_bits64(). > >> This case should be quite rare, since the bug > >> has not asserted itself. > >> > >> --- > >> It's attempt for speed optimization, but in the > >> process it turned out it needs also bugfixing. > >> > >> I only tested the old case of the code, > >> to confirm i've implemented the correct function. > >> > >> Haven't done any benchmarks or run fate. > > > > fate fails: > > > > Well, the good news is that > it compiles and doesn't crash. > > The change of generated files is expected. > The old code would write up to 63 bits > with regular put_bits() that is limited to 31 bits. > > Are av_assert2() enabled during fate runs? > (That's how put_bits() checks for bit length.) add --assert-level=2 if you want it checked > > > I'd like to know if my code is correct/incorrect > before attempting to changing fate checksums. the fate test looked like it did not decode the encoded data as half the checksums where gone instead of changed IIRC [...]
--- ./tests/ref/vsynth/vsynth1-vc2-420p 2018-02-27 23:48:20.527608799 +0100 +++ tests/data/fate/vsynth1-vc2-420p 2018-02-28 23:16:02.250764645 +0100 @@ -1,4 +1,2 @@ -fb8fffcfc17558c87dd11a67ccb0f615 *tests/data/fate/vsynth1-vc2-420p.mov +f24243a627788000de7147d38880d019 *tests/data/fate/vsynth1-vc2-420p.mov 1155415 tests/data/fate/vsynth1-vc2-420p.mov -387696707c79cf1a6c9aeff4024226b9 *tests/data/fate/vsynth1-vc2-420p.out.rawvideo -stddev: 0.00 PSNR:999.99 MAXDIFF: 0 bytes: 7603200/ 760320