[FFmpeg-devel] & vs. &&

[Benoit Fouet]

On 2009-10-12 17:22, Benoit Fouet wrote:
> On 2009-10-12 16:22, Reimar D?ffinger wrote:
>> On Mon, Oct 12, 2009 at 02:36:58PM +0100, John Cox wrote:
>>   
>>> Hi
>>>
>>>     
>>>> Hi,
>>>>
>>>> I (well, gcc) have (has) seen some weirdness on the use of & vs. && in
>>>> our codebase:
>>>>
>>>> first one:
>>>> libavformat/aviobuf.c:593: warning: suggest parentheses around operand
>>>> of ?!? or change ?&? to ?&&? or ?!? to ?~?
>>>> =========== ><8 ===========
>>>> int url_resetbuf(ByteIOContext *s, int flags)
>>>> {
>>>>    URLContext *h = s->opaque;
>>>>    if ((flags & URL_RDWR) || (h && h->flags != flags && !h->flags &
>>>> URL_RDWR))
>>>>        return AVERROR(EINVAL);
>>>> =========== 8>< ===========
>>>>
>>>> I don't understand what the intent of the test is, but am quite sure it
>>>> is wrong, !h->flags & URL_RDWR being always false.
>>>> I'd naively think the correct way would be to remove the '!', but am not
>>>> sure.
>>>>       
>>> I admit to not having read the surrounding code but I'd have thought the intent
>>> was:
>>>
>>>     if ((flags & URL_RDWR) || (h && h->flags != flags && !(h->flags &
>>> URL_RDWR)))
>>>     
>> Yes.
>>
>>   
>>> giving:
>>>
>>>     if ((flags & URL_RDWR) || (h && h->flags != flags))
>>>
>>> as the replacement code
>>>     
>> Huh? That would do something different than intended I think. It should
>> be possible to reset/reopen a read-write file both as read-only and as
>> write-only, which above code wouldn't allow.
>>   
> 
> thanks for the explanation.
> 
> So, I have a patch now:
> Index: libavformat/aviobuf.c
> ===================================================================
> --- libavformat/aviobuf.c       (revision 20209)
> +++ libavformat/aviobuf.c       (working copy)
> @@ -590,7 +590,7 @@ int url_setbufsize(ByteIOContext *s, int
>  int url_resetbuf(ByteIOContext *s, int flags)
>  {
>      URLContext *h = s->opaque;
> -    if ((flags & URL_RDWR) || (h && h->flags != flags && !h->flags &
> URL_RDWR))
> +    if ((flags & URL_RDWR) || (h && h->flags != flags && !(h->flags &
> URL_RDWR)))
>          return AVERROR(EINVAL);
> 
>      if (flags & URL_WRONLY) {
> 
> 
> This is crashing the following command line on my machine (triggered by
> make test), can anybody reproduce ?
> 
> $ gdb --args ./ffmpeg_g -v 0 -y -flags +bitexact -dct fastint -idct
> simple -sws_flags +accurate_rnd+bitexact -t 1 -qscale 10 -f image2
> -vcodec pgmyuv -i ./tests/vsynth1/%02d.pgm -f s16le -i
> ././tests/data/asynth1.sw -acodec mp2
> ././tests/data/b-lavf.nut                                                                                                                                                        
> 

The problem seems to be in the handling of buf_end.
When entering put_buffer() from libavformat/nutenc.c:389, the
ByteIOContext is as follows:
{buffer = 0xa465324 "\003",
 buffer_size = 1024,
 buf_ptr = 0xa465324 "\003",
 buf_end = 0x0,
 opaque = 0xa465310,
 read_packet = 0,
 write_packet = 0x806a580 <dyn_buf_write>,
 seek = 0x806a4f0 <dyn_buf_seek>,
 pos = 89,
 must_flush = 0,
 eof_reached = 0,
 write_flag = 0,
 is_streamed = 0,
 max_packet_size = 0,
 checksum = 0,
 checksum_ptr = 0x0,
 update_checksum = 0,
 error = 0,
 read_pause = 0,
 read_seek = 0}

I don't really have time to investigate a lot but the fact that buf_end
is not set seems to be our problem here, making the test len > size
fail, because len is negative, and doing a memcpy with a negative len
(which is often a bad idea :) )

Index: libavformat/aviobuf.c
===================================================================
@@ -108,7 +108,7 @@ void put_buffer(ByteIOContext *s, const

     while (size > 0) {
         len = (s->buf_end - s->buf_ptr);
         if (len > size) /* here, len is < 0 */
             len = size;
         memcpy(s->buf_ptr, buf, len);
         s->buf_ptr += len;

Ben