On Tue, Sep 02, 2014 at 01:13:27PM -0700, Peter Kasting wrote: > On Sat, Aug 30, 2014 at 2:21 AM, wm4 <nfx...@googlemail.com> wrote: > > I'd > > expect it rather to hide bugs than to expose them. For example, it > > could make a static analyzer with value range analysis stop working, > > because the casts will basically throw a wrench into its algorithm. > > > I can't understand how the sorts of casts that would be introduced here. > I'm suggesting would harm static analysis. If we're assigning a value to a > variable of type T, then "T var = x;" and "T var = (T)x;" have the same > effect, and a static analyzer ought to treat them identically.
The problem is that we have nothing that ensures the form T var = (T)x; over T2 var = (T)x; (where T2 is a wider type than T for example). Depending on the static analyzer you can make it ignore T var = x; but it will start warning again once the type of var or x changes. + // !!! Is this cast safe? + sub->end_display_time = (uint32_t)av_rescale_q(avpkt->duration, + avctx->pkt_timebase, ms); Don't really see anything much better to do. You could clamp it to e.g. INT_MAX, but not convinced it would get any better by that. + // !!! Are these casts safe? + s->base_matrix[0][i] = (uint8_t)vp31_intra_y_dequant[i]; + s->base_matrix[1][i] = (uint8_t)vp31_intra_c_dequant[i]; + s->base_matrix[2][i] = (uint8_t)vp31_inter_dequant[i]; That one is answered by a simple look in the table: None are negative, so yes. No idea if there is a specific reason they use signed types. Considering this seems to be the only place they are used, it probably is a "bug" in sofar as the declared types of the tables are stupid. secs %= 86400; - tm->tm_hour = secs / 3600; + tm->tm_hour =(int)(secs / 3600); tm->tm_min = (secs % 3600) / 60; - tm->tm_sec = secs % 60; + tm->tm_sec = secs % 60; The second part breaks the alignment. And the first one the compiler in theory could figure out on its own... +// !!! Should |pts_num| and |pts_den| be uint64_t? void avpriv_set_pts_info(AVStream *s, int pts_wrap_bits, unsigned int pts_num, unsigned int pts_den); I can only hope that nobody has/will come up with some insanity where that would be useful. + if (ffio_limit(pb, (int)length) != length) This one actually looks kind of like a bug, the cast might invoke undefined behaviour. @@ -1500,7 +1500,7 @@ static void matroska_add_index_entries(MatroskaDemuxContext *matroska) { EbmlList *index_list; MatroskaIndex *index; - int index_scale = 1; + uint64_t index_scale = 1; That looks like it will be a really expensive change, division by a 64 bit value isn't fast. In general, the nicer way to avoid the matroska issues and saving a bit of memory would be to add a EBML_UINT32 (dummy) type. -static int matroska_aac_sri(int samplerate) +static int matroska_aac_sri(double samplerate) I don't think this is equivalent at all. Not sure if it is better or worse though, we seem to switch between doing int and doing float comparisons. - avpriv_set_pts_info(st, 64, matroska->time_scale * track->time_scale, + avpriv_set_pts_info(st, 64, (unsigned int)(matroska->time_scale * track->time_scale), 1000 * 1000 * 1000); /* 64 bit pts in ns */ I see where the other question came from. That looks a bit annoying, won't work for time_scale values > 4s?? Not that I think anyone cares. --- a/libavformat/riffdec.c +++ b/libavformat/riffdec.c @@ -185,7 +185,7 @@ int ff_read_riff_info(AVFormatContext *s, int64_t size) while ((cur = avio_tell(pb)) >= 0 && cur <= end - 8 /* = tag + size */) { uint32_t chunk_code; - int64_t chunk_size; + unsigned int chunk_size; That seems risky to me. We seem to have the right checks right now, but just one mistake and we have an integer overflow in a malloc argument and a huge security hole if we using a 32 bit type here. if (bitrate >= 0 && bitrate <= INT_MAX) - ic->bit_rate = bitrate; + ic->bit_rate = (int)bitrate; I'd kind of think the compilers should manage at least these trivial cases... --- a/libavformat/wavdec.c +++ b/libavformat/wavdec.c @@ -61,7 +61,7 @@ typedef struct WAVDemuxContext { #if CONFIG_WAV_DEMUXER -static int64_t next_tag(AVIOContext *pb, uint32_t *tag) +static unsigned int next_tag(AVIOContext *pb, uint32_t *tag) { *tag = avio_rl32(pb); return avio_rl32(pb); @@ -76,10 +76,10 @@ static int64_t wav_seek_tag(WAVDemuxContext * wav, AVIOContext *s, int64_t offse } /* return the size of the found tag */ -static int64_t find_tag(WAVDemuxContext * wav, AVIOContext *pb, uint32_t tag1) +static unsigned int find_tag(WAVDemuxContext * wav, AVIOContext *pb, uint32_t tag1) { unsigned int tag; - int64_t size; + unsigned int size; for (;;) { if (avio_feof(pb)) These seem reasonable - int level = e->param[1] ? av_clip(eval_expr(p, e->param[1]), INT_MIN, INT_MAX) : AV_LOG_INFO; + int level = e->param[1] ? (int)av_clip64((int64_t)eval_expr(p, e->param[1]), INT_MIN, INT_MAX) : AV_LOG_INFO; ?? I think I've seen another one like it. Were they meant to be av_clipf/av_clipd? - int idx= av_clip(eval_expr(p, e->param[0]), 0, VARS-1); - uint64_t r= isnan(p->var[idx]) ? 0 : p->var[idx]; + int idx= av_clip((int)eval_expr(p, e->param[0]), 0, VARS-1); + uint64_t r= isnan(p->var[idx]) ? 0 : (uint64_t)p->var[idx]; r= r*1664525+1013904223; - p->var[idx]= r; + p->var[idx]= (double)r; Can this even work correctly?? I think we'd want to mask that to what double can store without losing precision. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel