[Ffmpeg-cvslog] r6355 - in trunk: libavcodec/4xm.c libavcodec/cyuv.c libavcodec/error_resilience.c libavcodec/h263.c libavcodec/h264.c libavcodec/indeo3.c libavcodec/mjpeg.c libavcodec/mpeg12.c libavcodec/mpeg12data.h libavcodec/mpegaudiodec.c libavcodec/mpegvideo.c libavcodec/mpegvideo.h libavcodec/msmpeg4.c libavcodec/parser.c libavcodec/svq1.c libavcodec/vc1.c libavcodec/vp3.c libavcodec/wmadec.c libavformat/amr.c libavformat/asf.c libavformat/avformat.h libavformat/avienc.c libavformat/matroska.c libavformat/mpeg.c libavformat/ogg2.c libavformat/ogg2.h libavformat/oggparsevorbis.c libavformat/rtp.c libavformat/rtpproto.c libavformat/udp.c

[Michael Niedermayer]

Hi

On Wed, Sep 27, 2006 at 10:32:50PM +0100, M?ns Rullg?rd wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> > Hi
> >
> > On Wed, Sep 27, 2006 at 09:47:42PM +0200, mru wrote:
> > [...]
> >> Modified: trunk/libavcodec/error_resilience.c
> >> ==============================================================================
> >> --- trunk/libavcodec/error_resilience.c	(original)
> >> +++ trunk/libavcodec/error_resilience.c	Wed Sep 27 21:47:39 2006
> >> @@ -70,7 +70,7 @@
> >>      }
> >>  }
> >>  
> >> -static void filter181(int16_t *data, int width, int height, int stride){
> >> +static void filter181(uint16_t *data, int width, int height, int stride){
> >
> > filter181 runs a (-1,8,-1) filter over the dc values which can make them
> > negative, so the array must be signed and that is the MpegEncContext.dc_val
> > array for which i already had a bad feeling when i saw your patch ...
> >
> > alternatively you can of course change the filter181 and related code 
> > but note, clipling <0 away is not ok though!
> 
> Then why didn't you say so when I asked several weeks ago?  

because i wasnt sure if it was ok to make it unsigend or not, i just
had a bad feeling about it, if i at that time knew of a specific case
which needed it signed i would of course have pointed to that


> It's not
> that easy trying to figure out which signedness is correct when it
> changes with every function call.

yes, so how should i? :)
anyway this isnt that bad, simply making filter181 and the stuff afterwards
use a temporary array will probably work or reverse the dc_val change or
completely rewrite the filter181 and related code


> 
> > [...]
> >> Modified: trunk/libavformat/mpeg.c
> >> ==============================================================================
> >> --- trunk/libavformat/mpeg.c	(original)
> >> +++ trunk/libavformat/mpeg.c	Wed Sep 27 21:47:39 2006
> >> @@ -1308,7 +1308,7 @@
> >>  
> >>  
> >>  typedef struct MpegDemuxContext {
> >> -    int header_state;
> >> +    int32_t header_state;
> >>      unsigned char psm_es_type[256];
> >>  } MpegDemuxContext;
> >>  
> >> @@ -1339,7 +1339,7 @@
> >>  }
> >>  
> >>  static int find_next_start_code(ByteIOContext *pb, int *size_ptr,
> >> -                                uint32_t *header_state)
> >> +                                int32_t *header_state)
> >
> > without reading the code id say the header_state should be uint32_t
> 
> It's tested for <0 somewhere...  I suppose it could be rewritten of
> course.

hmmm, where is header_state tested for <0 ?

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is