Thanks for the comments, I'll update my next patch to take that into account. But first I wanted to discuss your second point (regarding int64_t/uint64_t choice). I have actually looked at all places that use AVStream::id (34 files under libavformat/ + a few more files outside it). There are a few places which treat stream id as unsigned (e.g. libavformat/bink.c, which assigns AVStream::id to be the result of avio_rl32(pb) ). But most places either assign some signed int value to AVStream::id (the int value often comes as an input parameter to some function that actually assigns stream id) or use negative values for special cases (e.g. libavformat/swfdec.c, libavformat/wtvdec.c). Since most of the code expects AVStream::id to be signed for now, I've decided to make it an int64_t, it makes for a smaller/easier change. I have also been trying to figure out what's FFmpeg code style stance on doing something like 'typedef int64_t StreamId' and then using StreamId type whenever we deal with stream ids. But that's probably a more C++-style approach, not sure if it's appropriate here (and https://ffmpeg.org/developer.html doesn't seem to address this directly). Any opinions on this?
On Thu, Mar 10, 2016 at 12:19 AM, Nicolas George <geo...@nsup.org> wrote: > Thanks for the patch. > > Le decadi 20 ventôse, an CCXXIV, Sergey Volk a écrit : > > I have also bumped the major version to 58 locally in version.h, and > > re-ran make with the stream id being int64_t and fixed all new > > warnings that showed up (only saw new warnings related to the > > incorrect format being used for int64_t value). > > Commit messages are usually written in an impersonal form. Remember that > they will stay. That does not matter much. > > > av_log(avf, AV_LOG_VERBOSE, > > - "Match slave stream #%d with stream #%d id > 0x%x\n", > > - i, j, st->id); > > +#if FF_API_OLD_INT32_STREAM_ID > > + "Match slave stream #%d with stream #%d id > 0x%x\n" > > +#else > > + "Match slave stream #%d with stream #%d id > > 0x%"PRIx64"\n" > > +#endif > > + , i, j, st->id); > > You could do much simpler by casting the id unconditionally to int64_t: > > /* TODO remove cast after FF_API_OLD_INT32_STREAM_ID removal */ > av_log(... "0x%"PRIx64"\n", (int64_t)st->id); > > (I would put the comment at each place the cast is used, to ease finding > all > the casts that can be removed.) > > As a side note, I wonder if uint64_t would not be better than the signed > variant. > > Regards, > > -- > Nicolas George > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
