[FFmpeg-devel] [PATCH] remove unused and broken test program in libavutil/base64.c
Michael Niedermayer
michaelni
Thu Feb 5 21:35:25 CET 2009
On Thu, Feb 05, 2009 at 09:04:36PM +0100, Stefano Sabatini wrote:
> On date Sunday 2009-02-01 23:33:08 +0100, Michael Niedermayer encoded:
> > On Sun, Feb 01, 2009 at 07:06:21PM +0100, Stefano Sabatini wrote:
> > > On date Saturday 2009-01-31 22:08:26 +0100, Michael Niedermayer encoded:
> > > > On Sat, Jan 31, 2009 at 07:08:41PM +0100, Stefano Sabatini wrote:
> > > > > On date Saturday 2009-01-31 01:11:00 +0100, Michael Niedermayer encoded:
> > > > > > On Sat, Jan 31, 2009 at 12:26:20AM +0100, Stefano Sabatini wrote:
> > > > > > > On date Friday 2009-01-30 17:06:22 +0100, Michael Niedermayer encoded:
> > > > > > > > On Fri, Jan 30, 2009 at 12:46:36AM +0100, Stefano Sabatini wrote:
> > > > > > > [...]
> > > > > > > > > I think I addressed all the points.
> > > > > > > > >
> > > > > > > > > Round 5:
> > > > > > > > > ------------------8<----------------------8<-------------------------
> > > > > > > > > #ifdef TEST
> > > > > > > > > #include "mem.h"
> > > > > > > > > #include "lfg.h"
> > > > > > > > >
> > > > > > > > > #undef printf
> > > > > > > > >
> > > > > > > > > #define MAX_DATA_SIZE 1024
> > > > > > > > > #define MAX_ENCODED_SIZE 2048
> > > > > > > > >
> > > > > > > >
> > > > > > > > > #define SHOW_STUFF(stuff) show_stuff ? (const char *)stuff : "[...]"
> > > > > > > >
> > > > > > > > please explain wat this is supposed to be good for?
> > > > > > >
> > > > > > > Yes, here there is the first lines of the output:
> > > > > > >
> > > > > > > Encoding/decoding tests
> > > > > > > Encoding data with size 0 bytes ''... encoded as ''... passed!
> > > > > > > Encoding data with size 1 bytes '1'... encoded as 'MQ=='... passed!
> > > > > > > Encoding data with size 2 bytes '22'... encoded as 'MjI='... passed!
> > > > > > > Encoding data with size 3 bytes '333'... encoded as 'MzMz'... passed!
> > > > > > > Encoding data with size 4 bytes '4444'... encoded as 'NDQ0NA=='... passed!
> > > > > > > Encoding data with size 5 bytes '55555'... encoded as 'NTU1NTU='... passed!
> > > > > > > Encoding data with size 6 bytes '666666'... encoded as 'NjY2NjY2'... passed!
> > > > > > > Encoding data with size 7 bytes 'abc:def'... encoded as 'YWJjOmRlZg=='... passed!
> > > > > > > Encoding data with size 0 bytes '[...]'... encoded as '[...]'... passed!
> > > > > > > Encoding data with size 968 bytes '[...]'... encoded as '[...]'... passed!
> > > > > > > Encoding data with size 780 bytes '[...]'... encoded as '[...]'... passed!
> > > > > > > Encoding data with size 647 bytes '[...]'... encoded as '[...]'... passed!
> > > > > > > ...
> > > > > > >
> > > > > > > For the data with a reference encoded string I think it is a good idea
> > > > > > > to show how the data is encoded, in case of problem the hacker should
> > > > > > > be able to immediately spot where the problem is.
> > > > > > >
> > > > > > > For the other ones the length of the encoded string is usually too
> > > > > > > long and it is too awkward to display it, but no strong opinion on
> > > > > > > this, just tell how do you prefer it.
> > > > > >
> > > > > > hmm ok, ive not conciously registererd that this was run with several hundread
> > > > > > bytes.
> > > > > > >
> > > > > > > > [...]
> > > > > > > > > printf("\nDecoding tests on invalid data\n");
> > > > > > > > > {
> > > > > > > > > uint8_t data[32];
> > > > > > > > > const char *encoded[] = { "M", "M=M=", "MQ===" };
> > > > > > > >
> > > > > > > > why is this splited off and not in the main table?
> > > > > > >
> > > > > > > I believe this test cannot be reduced to the previous one, here we're
> > > > > > > just *decoding* and checking for validity.
> > > > > >
> > > > > > if(A) A -> decode, cmp B
> > > > > > if(B) B -> encode, cmp A
> > > > > >
> > > > > > B being NULL
> > > > > >
> > > > > > am i missing something?
> > > > >
> > > > > The test is an encoding/decoding test, not a decoding/encoding test,
> > > > > we could maybe find some trick to make encode_decode_test() act as a
> > > > > decoding invalidity test, but this:
> > > > >
> > > > > 1) would increase complexity of a program meant to be simple
> > > > > 2) it's not worth the trouble
> > > >
> > > > a if(B) is hardly more complex than to duplicate the whole check
> > >
> > > Well we have:
> > > test_encode_decode:
> > > D -> E, cmp (E, E'), E -> D', cmp (D, D')
> > >
> > > the second test is:
> > > E -> D, if decode successfull -> fail
> >
> > if(D){
> > D->E'
> > if(cmp(E, E'))
> > fail
> > }
> >
> > E->D'
> > if(cmp(D, D'))
> > fail
> >
> > cmp(A,B){
> > if((!A) != (!B)) return -1
> > else if(A) return strcmp(A,B)
> > }
> >
> > anyway if this turns out more complex, forget it
>
> Yes, I'd prefer to keep the current solution, which I find simpler and
> possibly more readable.
>
> > > I cannot see an immediate way to reduce the second test to the first
> > > one, if there is I think it would most likely make the code more
> > > complex, maybe I'm wrong.
> > >
> > > > also about SHOW_STUFF() it shouldnt be there and you shouldnt print
> > > > anything at all if things are ok, just print what does not produce the
> > > > expected output or an OK at the end
> > > > if a 1kb case mismatches it should be printed as well
> > >
> > > OK, done, but printing data will show non-printable chars, is this a
> > > problem?
> >
> > well, of course, printing hex maybe better for debuging anyway.
>
> Yes. And while we're at it what about to move av_hex_dump() from lavf
> to lavu?
av_hex_dump() should be deprecated maybe (it does not seem usefull for us)
av_hex_dump_log() should be cleaned up and simplified
after that lavu can be considered
>
> That's a nice function to have in libavutil, in this case it would
> have saved its reimplementation in this test program.
>
> > also i think we are wasting time on this rather pointless piece of code
> > i guess thats my fault :(
>
> I enjoy your remarks most of the time, but I agree there are more
> urgent/interesting problems to solve out there! ;-)
considering our agreement on the irrelevnace of the code
please remove the tests for invalidness, remove the hex dumping and
try to remove as much of the other code as well.
also either the fixed vectors or the random ones must be dropped
they are redundant in some sense (keep whats less code)
its just a simple self test, what you wrote is overkill IMHO
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> ... defining _GNU_SOURCE...
For the love of all that is holy, and some that is not, don't do that.
-- Luca & Mans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090205/713ec8a8/attachment.pgp>
More information about the ffmpeg-devel
mailing list