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

[Sergey Volk]

Yeah, I was using Gmail web interface, it does that. I'll try attaching the
patch file next time.

On Thu, Mar 10, 2016 at 1:23 AM, Moritz Barsnick  wrote:

> On Wed, Mar 09, 2016 at 15:56:53 -0800, Sergey Volk wrote:
> > -if (fmt_ctx->iformat->flags & AVFMT_SHOW_IDS) print_fmt("id",
> > "0x%x", stream->id);
> > +#if FF_API_OLD_INT32_STREAM_ID
> > +#define STREAM_ID_FORMAT "0x%x"
> > +#else
> > +#define STREAM_ID_FORMAT "0x%"PRIx64
> > +#endif
> > +if (fmt_ctx->iformat->flags & AVFMT_SHOW_IDS) print_fmt("id",
> > STREAM_ID_FORMAT, stream->id);
> > +#undef STREAM_ID_FORMAT
>
> From pure visual inspection, I believe your patch got broken (wrapped
> lines) by your mailer agent or something along the line.
>
> Moritz
> ___
> 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

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

[Moritz Barsnick]

On Wed, Mar 09, 2016 at 15:56:53 -0800, Sergey Volk wrote:
> -if (fmt_ctx->iformat->flags & AVFMT_SHOW_IDS) print_fmt("id",
> "0x%x", stream->id);
> +#if FF_API_OLD_INT32_STREAM_ID
> +#define STREAM_ID_FORMAT "0x%x"
> +#else
> +#define STREAM_ID_FORMAT "0x%"PRIx64
> +#endif
> +if (fmt_ctx->iformat->flags & AVFMT_SHOW_IDS) print_fmt("id",
> STREAM_ID_FORMAT, stream->id);
> +#undef STREAM_ID_FORMAT

From pure visual inspection, I believe your patch got broken (wrapped
lines) by your mailer agent or something along the line.

Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Nicolas George]

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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sergey Volk]

From: Sergey Volk 
Date: Wed, 9 Mar 2016 14:34:19 -0800
Subject: [PATCH] Change AVStream::id to int64_t in the next version bump

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).
---
 ffprobe.c   | 8 +++-
 libavformat/avformat.h  | 4 
 libavformat/concatdec.c | 8 ++--
 libavformat/dump.c  | 4 
 libavformat/mpegtsenc.c | 7 ++-
 libavformat/version.h   | 3 +++
 6 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/ffprobe.c b/ffprobe.c
index f7b51ad..21eab61 100644
--- a/ffprobe.c
+++ b/ffprobe.c
@@ -2287,7 +2287,13 @@ static int show_stream(WriterContext *w,
AVFormatContext *fmt_ctx, int stream_id
 }
 }

-if (fmt_ctx->iformat->flags & AVFMT_SHOW_IDS) print_fmt("id",
"0x%x", stream->id);
+#if FF_API_OLD_INT32_STREAM_ID
+#define STREAM_ID_FORMAT "0x%x"
+#else
+#define STREAM_ID_FORMAT "0x%"PRIx64
+#endif
+if (fmt_ctx->iformat->flags & AVFMT_SHOW_IDS) print_fmt("id",
STREAM_ID_FORMAT, stream->id);
+#undef STREAM_ID_FORMAT
 else  print_str_opt("id", "N/A");
 print_q("r_frame_rate",   stream->r_frame_rate,   '/');
 print_q("avg_frame_rate", stream->avg_frame_rate, '/');
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index a558f2d..253b293 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -871,7 +871,11 @@ typedef struct AVStream {
  * decoding: set by libavformat
  * encoding: set by the user, replaced by libavformat if left unset
  */
+#if FF_API_OLD_INT32_STREAM_ID
 int id;
+#else
+int64_t id;
+#endif
 /**
  * Codec context associated with this stream. Allocated and freed by
  * libavformat.
diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index e69096f..481c8433 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -238,8 +238,12 @@ static int match_streams_exact_id(AVFormatContext *avf)
 for (j = 0; j < avf->nb_streams; j++) {
 if (avf->streams[j]->id == st->id) {
 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);
 if ((ret = copy_stream_props(avf->streams[j], st)) < 0)
 return ret;
 cat->cur_file->streams[i].out_stream_index = j;
diff --git a/libavformat/dump.c b/libavformat/dump.c
index 86bb82d..8b50ec1 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -453,7 +453,11 @@ static void dump_stream_format(AVFormatContext *ic, int i,
 /* the pid is an important information, so we display it */
 /* XXX: add a generic system */
 if (flags & AVFMT_SHOW_IDS)
+#if FF_API_OLD_INT32_STREAM_ID
 av_log(NULL, AV_LOG_INFO, "[0x%x]", st->id);
+#else
+av_log(NULL, AV_LOG_INFO, "[0x%"PRIx64"]", st->id);
+#endif
 if (lang)
 av_log(NULL, AV_LOG_INFO, "(%s)", lang->value);
 av_log(NULL, AV_LOG_DEBUG, ", %d, %d/%d", st->codec_info_nb_frames,
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index 68f9867..0244b7f 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -833,7 +833,12 @@ static int mpegts_init(AVFormatContext *s)
 ts_st->pid = st->id;
 } else {
 av_log(s, AV_LOG_ERROR,
-   "Invalid stream id %d, must be less than 8191\n", st->id);
+#if FF_API_OLD_INT32_STREAM_ID
+   "Invalid stream id %d, must be less than 8191\n",
+#else
+   "Invalid stream id %"PRId64", must be less than 8191\n",
+#endif
+st->id);
 ret = AVERROR(EINVAL);
 goto fail;
 }
diff --git a/libavformat/version.h b/libavformat/version.h
index 7dcce2c..e0ac45a 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -74,6 +74,9 @@
 #ifndef FF_API_OLD_OPEN_CALLBACKS
 #define FF_API_OLD_OPEN_CALLBACKS   (LIBAVFORMAT_VERSION_MAJOR < 58)
 #endif
+#ifndef FF_API_OLD_INT32_STREAM_ID
+#define FF_API_OLD_INT32_STREAM_ID  (LIBAVFORMAT_VERSION_MAJOR < 58)
+#endif

 #ifndef FF_API_R_FRAME_RATE
 #define FF_API_R_FRAME_RATE1
-- 
2.7.0.rc3.207.g0ac5344

On Wed, Mar 9, 2016 at 3:55 PM, Sergey Volk  wrote:
> Ok, so then I guess we'll need to update AVStream::id to be 64-bit
> first, and then I'll make the necessary changes in matroskadec.
> I've prepared a patch to bump AVStream::id to be int64_t in the next
> major version, I'll send it out shortly. After I rebuilt 

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

[Sergey Volk]

Ok, so then I guess we'll need to update AVStream::id to be 64-bit
first, and then I'll make the necessary changes in matroskadec.
I've prepared a patch to bump AVStream::id to be int64_t in the next
major version, I'll send it out shortly. After I rebuilt ffmpeg
with AVStream::id being int64_t I got a couple of new warnings in the
code that was using 32-bit format specifiers for printing
stream ids, I've fixed those as well. I've also re-ran 'make fate' and
all the tests seem to be good.

On Sat, Mar 5, 2016 at 2:47 AM, Michael Niedermayer
 wrote:
> On Fri, Mar 04, 2016 at 04:19:18PM -0800, Sergey Volk wrote:
>> Ok, something like this for now, then?
>
> your original patch contained a nice commit message, this one
> doesnt
>
>
>> I'm new to ffmpeg development. When is the next version bump going to happen?
>
> you can make changes at the next bump by using #if FF_API...
> see libavfilter/version.h
>
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> It is dangerous to be right in matters on which the established authorities
> are wrong. -- Voltaire
>
> ___
> 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

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

[Michael Niedermayer]

On Fri, Mar 04, 2016 at 04:19:18PM -0800, Sergey Volk wrote:
> Ok, something like this for now, then?

your original patch contained a nice commit message, this one
doesnt


> I'm new to ffmpeg development. When is the next version bump going to happen?

you can make changes at the next bump by using #if FF_API...
see libavfilter/version.h


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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sergey Volk]

Ok, something like this for now, then?
I'm new to ffmpeg development. When is the next version bump going to happen?
---
 libavformat/matroskadec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d20568c..4c3e53a 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1856,6 +1856,9 @@ static int matroska_parse_tracks(AVFormatContext *s)
 return AVERROR(ENOMEM);
 }

+if (track->num <= INT_MAX)
+  st->id = (int) track->num;
+
 if (key_id_base64) {
 /* export encryption key id as base64 metadata tag */
 av_dict_set(>metadata, "enc_key_id", key_id_base64, 0);
-- 
2.7.0.rc3.207.g0ac5344

On Thu, Mar 3, 2016 at 2:14 AM, Carl Eugen Hoyos  wrote:
> wm4  googlemail.com> writes:
>
>> > +st->id = (int) track->num;
>
>> Might be better after all not to set the id if it's out of range?
>
> Yes, please.
>
> While there, the id field could be changed to 64bit with the
> next version bump.
>
> Carl Eugen
>
> ___
> 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

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

[Carl Eugen Hoyos]

wm4  googlemail.com> writes:

> > +st->id = (int) track->num;

> Might be better after all not to set the id if it's out of range?

Yes, please.

While there, the id field could be changed to 64bit with the 
next version bump.

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Wed, 2 Mar 2016 15:29:21 -0800
Sergey Volk  wrote:

> As far as I can see FFmpeg currently doesn't set AVStream::id for
> matroska/webm streams. I think we could use either MatroskaTrack::num
> (TrackNumber) or MatroskaTrack::uid (TrackUID) for that.
> I have found a few discussions claiming that TrackUID could be missing,
> even though TrackUID is marked as mandatory field in matroska spec, for
> example see
> https://github.com/mbunkus/mkvtoolnix/issues/1050
> https://lists.w3.org/Archives/Public/public-inbandtracks/2014May/0003.html
> So I guess it's safer to use TrackNumber for now.
> ---
>  libavformat/matroskadec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index d20568c..8b80df1 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1856,6 +1856,8 @@ static int matroska_parse_tracks(AVFormatContext *s)
>  return AVERROR(ENOMEM);
>  }
> 
> +st->id = (int) track->num;
> +
>  if (key_id_base64) {
>  /* export encryption key id as base64 metadata tag */
>  av_dict_set(>metadata, "enc_key_id", key_id_base64, 0);

The int cast makes me suspicious. It might be ok to discard the
additional bits for pathological files, but I don't quite remember
whether or not this can trigger UB.

Might be better after all not to set the id if it's out of range?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sergey Volk]

As far as I can see FFmpeg currently doesn't set AVStream::id for
matroska/webm streams. I think we could use either MatroskaTrack::num
(TrackNumber) or MatroskaTrack::uid (TrackUID) for that.
I have found a few discussions claiming that TrackUID could be missing,
even though TrackUID is marked as mandatory field in matroska spec, for
example see
https://github.com/mbunkus/mkvtoolnix/issues/1050
https://lists.w3.org/Archives/Public/public-inbandtracks/2014May/0003.html
So I guess it's safer to use TrackNumber for now.
---
 libavformat/matroskadec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d20568c..8b80df1 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1856,6 +1856,8 @@ static int matroska_parse_tracks(AVFormatContext *s)
 return AVERROR(ENOMEM);
 }

+st->id = (int) track->num;
+
 if (key_id_base64) {
 /* export encryption key id as base64 metadata tag */
 av_dict_set(>metadata, "enc_key_id", key_id_base64, 0);
-- 
2.7.0.rc3.207.g0ac5344
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel