[FFmpeg-devel] [PATCH] Use matroska TrackNumber for populating AVStream::id

[Sergey Volk]

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 <george at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>