diff mbox series

[FFmpeg-devel] ffmpeg: Carry streamid as metadata key 'id'

Message ID 71fccb5a1e28dfd23e3dd710c86c8a3a662bbd61.camel@haerdin.se
State New
Headers show
Series [FFmpeg-devel] ffmpeg: Carry streamid as metadata key 'id' | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Tomas Härdin April 12, 2024, 9:40 a.m. UTC 
This idea could be extended to other fields not presently considered to
be metadata, that would be handy to treat as such.

I use the key "id" because ffprobe outputs id= for streamid. Another
option could be to collect these types of metadata that go into
AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
something, then delete all of them using AV_DICT_IGNORE_SUFFIX near the
end of of_open() since they're for internal ffmpeg use.

The FATE change is just because av_dict() changes the order of things
when elements are deleted.

/Tomas

Comments

Michael Niedermayer April 12, 2024, 11:25 p.m. UTC | #1 
On Fri, Apr 12, 2024 at 11:40:47AM +0200, Tomas Härdin wrote:
> This idea could be extended to other fields not presently considered to
> be metadata, that would be handy to treat as such.
> 
> I use the key "id" because ffprobe outputs id= for streamid. Another
> option could be to collect these types of metadata that go into
> AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> something, then delete all of them using AV_DICT_IGNORE_SUFFIX near the
> end of of_open() since they're for internal ffmpeg use.
> 
> The FATE change is just because av_dict() changes the order of things
> when elements are deleted.
> 
> /Tomas

>  fftools/ffmpeg_demux.c              |    5 +++
>  fftools/ffmpeg_mux_init.c           |   56 ++++++++++++++++++++++++++----------
>  tests/ref/fate/matroska-stereo_mode |    6 +--
>  3 files changed, 49 insertions(+), 18 deletions(-)
> cd526b2292b6d7e3fb5739a04cf17fbe5f207f16  0001-ffmpeg-Carry-streamid-as-metadata-key-id.patch
> From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> Date: Fri, 12 Apr 2024 10:34:12 +0200
> Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> 
> This allows using -map_metadata and -metadata to copy/set streamids (PIDs).
> ---
>  fftools/ffmpeg_demux.c              |  5 +++
>  fftools/ffmpeg_mux_init.c           | 56 +++++++++++++++++++++--------
>  tests/ref/fate/matroska-stereo_mode |  6 ++--
>  3 files changed, 49 insertions(+), 18 deletions(-)

breaks:

./ffmpeg -i ~/videos/mm-short.mpg -vstats_file /tmp/vstats -bitexact -map 0:1 -map 0:1 -map 0:2 -threads 6 -vcodec libx264 -vcodec libx264 -pass 1 -b:v:0 300k -b:v:1 900k -passlogfile:v:1 /tmp/video2 -passlogfile:v:0 /tmp/video1 -t 1 -y -ab 128k  /tmp/x.ts

[mpegts @ 0x558d5e3b2140] Duplicate stream id 480
[out#0/mpegts @ 0x558d5e3b2000] Could not write header (incorrect codec parameters ?): Invalid argument
[vf#0:0 @ 0x558d5e400400] Error sending frames to consumers: Invalid argument
[vf#0:0 @ 0x558d5e400400] Task finished with error code: -22 (Invalid argument)
[vf#0:0 @ 0x558d5e400400] Terminating thread with return code -22 (Invalid argument)
[out#0/mpegts @ 0x558d5e3b2000] Nothing was written into output file, because at least one of its streams received no packets.

[...]
Tomas Härdin April 15, 2024, 8:35 a.m. UTC | #2 
lör 2024-04-13 klockan 01:25 +0200 skrev Michael Niedermayer:
> On Fri, Apr 12, 2024 at 11:40:47AM +0200, Tomas Härdin wrote:
> > This idea could be extended to other fields not presently
> > considered to
> > be metadata, that would be handy to treat as such.
> > 
> > I use the key "id" because ffprobe outputs id= for streamid.
> > Another
> > option could be to collect these types of metadata that go into
> > AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> > something, then delete all of them using AV_DICT_IGNORE_SUFFIX near
> > the
> > end of of_open() since they're for internal ffmpeg use.
> > 
> > The FATE change is just because av_dict() changes the order of
> > things
> > when elements are deleted.
> > 
> > /Tomas
> 
> >  fftools/ffmpeg_demux.c              |    5 +++
> >  fftools/ffmpeg_mux_init.c           |   56
> > ++++++++++++++++++++++++++----------
> >  tests/ref/fate/matroska-stereo_mode |    6 +--
> >  3 files changed, 49 insertions(+), 18 deletions(-)
> > cd526b2292b6d7e3fb5739a04cf17fbe5f207f16  0001-ffmpeg-Carry-
> > streamid-as-metadata-key-id.patch
> > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00
> > 2001
> > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> > 
> > This allows using -map_metadata and -metadata to copy/set streamids
> > (PIDs).
> > ---
> >  fftools/ffmpeg_demux.c              |  5 +++
> >  fftools/ffmpeg_mux_init.c           | 56 +++++++++++++++++++++----
> > ----
> >  tests/ref/fate/matroska-stereo_mode |  6 ++--
> >  3 files changed, 49 insertions(+), 18 deletions(-)
> 
> breaks:
> 
> ./ffmpeg -i ~/videos/mm-short.mpg -vstats_file /tmp/vstats -bitexact
> -map 0:1 -map 0:1 -map 0:2 -threads 6 -vcodec libx264 -vcodec libx264
> -pass 1 -b:v:0 300k -b:v:1 900k -passlogfile:v:1 /tmp/video2 -
> passlogfile:v:0 /tmp/video1 -t 1 -y -ab 128k  /tmp/x.ts
> 
> [mpegts @ 0x558d5e3b2140] Duplicate stream id 480

It's hardly strange if you map the same stream to the output twice that
you get duplicate streamids

/Tomas
Michael Niedermayer April 16, 2024, 12:30 a.m. UTC | #3 
On Mon, Apr 15, 2024 at 10:35:44AM +0200, Tomas Härdin wrote:
> lör 2024-04-13 klockan 01:25 +0200 skrev Michael Niedermayer:
> > On Fri, Apr 12, 2024 at 11:40:47AM +0200, Tomas Härdin wrote:
> > > This idea could be extended to other fields not presently
> > > considered to
> > > be metadata, that would be handy to treat as such.
> > > 
> > > I use the key "id" because ffprobe outputs id= for streamid.
> > > Another
> > > option could be to collect these types of metadata that go into
> > > AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> > > something, then delete all of them using AV_DICT_IGNORE_SUFFIX near
> > > the
> > > end of of_open() since they're for internal ffmpeg use.
> > > 
> > > The FATE change is just because av_dict() changes the order of
> > > things
> > > when elements are deleted.
> > > 
> > > /Tomas
> > 
> > >  fftools/ffmpeg_demux.c              |    5 +++
> > >  fftools/ffmpeg_mux_init.c           |   56
> > > ++++++++++++++++++++++++++----------
> > >  tests/ref/fate/matroska-stereo_mode |    6 +--
> > >  3 files changed, 49 insertions(+), 18 deletions(-)
> > > cd526b2292b6d7e3fb5739a04cf17fbe5f207f16  0001-ffmpeg-Carry-
> > > streamid-as-metadata-key-id.patch
> > > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00
> > > 2001
> > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> > > 
> > > This allows using -map_metadata and -metadata to copy/set streamids
> > > (PIDs).
> > > ---
> > >  fftools/ffmpeg_demux.c              |  5 +++
> > >  fftools/ffmpeg_mux_init.c           | 56 +++++++++++++++++++++----
> > > ----
> > >  tests/ref/fate/matroska-stereo_mode |  6 ++--
> > >  3 files changed, 49 insertions(+), 18 deletions(-)
> > 
> > breaks:
> > 
> > ./ffmpeg -i ~/videos/mm-short.mpg -vstats_file /tmp/vstats -bitexact
> > -map 0:1 -map 0:1 -map 0:2 -threads 6 -vcodec libx264 -vcodec libx264
> > -pass 1 -b:v:0 300k -b:v:1 900k -passlogfile:v:1 /tmp/video2 -
> > passlogfile:v:0 /tmp/video1 -t 1 -y -ab 128k  /tmp/x.ts
> > 
> > [mpegts @ 0x558d5e3b2140] Duplicate stream id 480
> 
> It's hardly strange if you map the same stream to the output twice that
> you get duplicate streamids

ok but asking for a stream to be mapped twice is a valid case.
Ideally FFmpeg should not fail, it should resolve all parameters
within what is valid.
It could fail if the user explcicitly asks for invalid parameters ...

thx

[...]
James Almer April 16, 2024, 12:33 a.m. UTC | #4 
On 4/15/2024 9:30 PM, Michael Niedermayer wrote:
> On Mon, Apr 15, 2024 at 10:35:44AM +0200, Tomas Härdin wrote:
>> lör 2024-04-13 klockan 01:25 +0200 skrev Michael Niedermayer:
>>> On Fri, Apr 12, 2024 at 11:40:47AM +0200, Tomas Härdin wrote:
>>>> This idea could be extended to other fields not presently
>>>> considered to
>>>> be metadata, that would be handy to treat as such.
>>>>
>>>> I use the key "id" because ffprobe outputs id= for streamid.
>>>> Another
>>>> option could be to collect these types of metadata that go into
>>>> AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
>>>> something, then delete all of them using AV_DICT_IGNORE_SUFFIX near
>>>> the
>>>> end of of_open() since they're for internal ffmpeg use.
>>>>
>>>> The FATE change is just because av_dict() changes the order of
>>>> things
>>>> when elements are deleted.
>>>>
>>>> /Tomas
>>>
>>>>   fftools/ffmpeg_demux.c              |    5 +++
>>>>   fftools/ffmpeg_mux_init.c           |   56
>>>> ++++++++++++++++++++++++++----------
>>>>   tests/ref/fate/matroska-stereo_mode |    6 +--
>>>>   3 files changed, 49 insertions(+), 18 deletions(-)
>>>> cd526b2292b6d7e3fb5739a04cf17fbe5f207f16  0001-ffmpeg-Carry-
>>>> streamid-as-metadata-key-id.patch
>>>>  From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00
>>>> 2001
>>>> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
>>>> Date: Fri, 12 Apr 2024 10:34:12 +0200
>>>> Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
>>>>
>>>> This allows using -map_metadata and -metadata to copy/set streamids
>>>> (PIDs).
>>>> ---
>>>>   fftools/ffmpeg_demux.c              |  5 +++
>>>>   fftools/ffmpeg_mux_init.c           | 56 +++++++++++++++++++++----
>>>> ----
>>>>   tests/ref/fate/matroska-stereo_mode |  6 ++--
>>>>   3 files changed, 49 insertions(+), 18 deletions(-)
>>>
>>> breaks:
>>>
>>> ./ffmpeg -i ~/videos/mm-short.mpg -vstats_file /tmp/vstats -bitexact
>>> -map 0:1 -map 0:1 -map 0:2 -threads 6 -vcodec libx264 -vcodec libx264
>>> -pass 1 -b:v:0 300k -b:v:1 900k -passlogfile:v:1 /tmp/video2 -
>>> passlogfile:v:0 /tmp/video1 -t 1 -y -ab 128k  /tmp/x.ts
>>>
>>> [mpegts @ 0x558d5e3b2140] Duplicate stream id 480
>>
>> It's hardly strange if you map the same stream to the output twice that
>> you get duplicate streamids
> 
> ok but asking for a stream to be mapped twice is a valid case.
> Ideally FFmpeg should not fail, it should resolve all parameters
> within what is valid.
> It could fail if the user explcicitly asks for invalid parameters ...

mpegts muxer could change the duplicate id for a free one. Afaik the 
field is documented as being modifiable by muxers. It could also do it 
internally.
Anton Khirnov April 16, 2024, 12:38 p.m. UTC | #5 
Quoting Tomas Härdin (2024-04-12 11:40:47)
> This idea could be extended to other fields not presently considered to
> be metadata, that would be handy to treat as such.
> 
> I use the key "id" because ffprobe outputs id= for streamid. Another
> option could be to collect these types of metadata that go into
> AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> something, then delete all of them using AV_DICT_IGNORE_SUFFIX near the
> end of of_open() since they're for internal ffmpeg use.
> 
> The FATE change is just because av_dict() changes the order of things
> when elements are deleted.
> 
> /Tomas
> 
> From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> Date: Fri, 12 Apr 2024 10:34:12 +0200
> Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> 
> This allows using -map_metadata and -metadata to copy/set streamids (PIDs).

I dislike this patch, metadata is the wrong place for such information.
James Almer April 16, 2024, 12:52 p.m. UTC | #6 
On Tue, Apr 16, 2024 at 9:38 AM Anton Khirnov <anton@khirnov.net> wrote:

> Quoting Tomas Härdin (2024-04-12 11:40:47)
> > This idea could be extended to other fields not presently considered to
> > be metadata, that would be handy to treat as such.
> >
> > I use the key "id" because ffprobe outputs id= for streamid. Another
> > option could be to collect these types of metadata that go into
> > AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> > something, then delete all of them using AV_DICT_IGNORE_SUFFIX near the
> > end of of_open() since they're for internal ffmpeg use.
> >
> > The FATE change is just because av_dict() changes the order of things
> > when elements are deleted.
> >
> > /Tomas
> >
> > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> >
> > This allows using -map_metadata and -metadata to copy/set streamids
> (PIDs).
>
> I dislike this patch, metadata is the wrong place for such information.
>

Can it be propagated within the InputStream?


>
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Tomas Härdin April 16, 2024, 2 p.m. UTC | #7 
mån 2024-04-15 klockan 21:33 -0300 skrev James Almer:
> On 4/15/2024 9:30 PM, Michael Niedermayer wrote:
> > On Mon, Apr 15, 2024 at 10:35:44AM +0200, Tomas Härdin wrote:
> > > lör 2024-04-13 klockan 01:25 +0200 skrev Michael Niedermayer:
> > > > On Fri, Apr 12, 2024 at 11:40:47AM +0200, Tomas Härdin wrote:
> > > > > This idea could be extended to other fields not presently
> > > > > considered to
> > > > > be metadata, that would be handy to treat as such.
> > > > > 
> > > > > I use the key "id" because ffprobe outputs id= for streamid.
> > > > > Another
> > > > > option could be to collect these types of metadata that go
> > > > > into
> > > > > AVStream fields under a namespace like FFMPEG: or AVSTREAM:
> > > > > or
> > > > > something, then delete all of them using
> > > > > AV_DICT_IGNORE_SUFFIX near
> > > > > the
> > > > > end of of_open() since they're for internal ffmpeg use.
> > > > > 
> > > > > The FATE change is just because av_dict() changes the order
> > > > > of
> > > > > things
> > > > > when elements are deleted.
> > > > > 
> > > > > /Tomas
> > > > 
> > > > >   fftools/ffmpeg_demux.c              |    5 +++
> > > > >   fftools/ffmpeg_mux_init.c           |   56
> > > > > ++++++++++++++++++++++++++----------
> > > > >   tests/ref/fate/matroska-stereo_mode |    6 +--
> > > > >   3 files changed, 49 insertions(+), 18 deletions(-)
> > > > > cd526b2292b6d7e3fb5739a04cf17fbe5f207f16  0001-ffmpeg-Carry-
> > > > > streamid-as-metadata-key-id.patch
> > > > >  From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17
> > > > > 00:00:00
> > > > > 2001
> > > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > > > > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > > > > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> > > > > 
> > > > > This allows using -map_metadata and -metadata to copy/set
> > > > > streamids
> > > > > (PIDs).
> > > > > ---
> > > > >   fftools/ffmpeg_demux.c              |  5 +++
> > > > >   fftools/ffmpeg_mux_init.c           | 56
> > > > > +++++++++++++++++++++----
> > > > > ----
> > > > >   tests/ref/fate/matroska-stereo_mode |  6 ++--
> > > > >   3 files changed, 49 insertions(+), 18 deletions(-)
> > > > 
> > > > breaks:
> > > > 
> > > > ./ffmpeg -i ~/videos/mm-short.mpg -vstats_file /tmp/vstats -
> > > > bitexact
> > > > -map 0:1 -map 0:1 -map 0:2 -threads 6 -vcodec libx264 -vcodec
> > > > libx264
> > > > -pass 1 -b:v:0 300k -b:v:1 900k -passlogfile:v:1 /tmp/video2 -
> > > > passlogfile:v:0 /tmp/video1 -t 1 -y -ab 128k  /tmp/x.ts
> > > > 
> > > > [mpegts @ 0x558d5e3b2140] Duplicate stream id 480
> > > 
> > > It's hardly strange if you map the same stream to the output
> > > twice that
> > > you get duplicate streamids
> > 
> > ok but asking for a stream to be mapped twice is a valid case.
> > Ideally FFmpeg should not fail, it should resolve all parameters
> > within what is valid.
> > It could fail if the user explcicitly asks for invalid parameters
> > ...
> 
> mpegts muxer could change the duplicate id for a free one. Afaik the 
> field is documented as being modifiable by muxers. It could also do
> it 
> internally.

That sounds like business logic that belongs further up than in a
muxer. That way more muxers that depend on st->id benefit from it.
ffmpeg_mux_init.c could warn about it and make up unique id's. Default
values could be passed upward via AVOptions

/Tomas
Tomas Härdin April 16, 2024, 2:12 p.m. UTC | #8 
tis 2024-04-16 klockan 09:52 -0300 skrev James Almer:
> On Tue, Apr 16, 2024 at 9:38 AM Anton Khirnov <anton@khirnov.net>
> wrote:
> 
> > Quoting Tomas Härdin (2024-04-12 11:40:47)
> > > This idea could be extended to other fields not presently
> > > considered to
> > > be metadata, that would be handy to treat as such.
> > > 
> > > I use the key "id" because ffprobe outputs id= for streamid.
> > > Another
> > > option could be to collect these types of metadata that go into
> > > AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> > > something, then delete all of them using AV_DICT_IGNORE_SUFFIX
> > > near the
> > > end of of_open() since they're for internal ffmpeg use.
> > > 
> > > The FATE change is just because av_dict() changes the order of
> > > things
> > > when elements are deleted.
> > > 
> > > /Tomas
> > > 
> > > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00
> > > 2001
> > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> > > 
> > > This allows using -map_metadata and -metadata to copy/set
> > > streamids
> > (PIDs).
> > 
> > I dislike this patch, metadata is the wrong place for such
> > information.

Seems like a matter of taste to me, but maybe I'm missing something

In the very common case where users want to copy PIDs from inputs to
outputs, implementing -map_streamid seems a bit silly. Consider also
the case where the user wants to copy codec and bitrate from some
source stream, such as when filtering audio. It would be nice if such
operations were handled by a common mechanism. Call it -map_params
perhaps

> 
> Can it be propagated within the InputStream?

This is what my earlier patch ([PATCH] ffmpeg: Add -copystreamid) did,
grabbing them from ist->st->id

/Tomas
Anton Khirnov July 1, 2024, 2:51 p.m. UTC | #9 
Quoting Tomas Härdin (2024-04-16 16:12:13)
> tis 2024-04-16 klockan 09:52 -0300 skrev James Almer:
> > On Tue, Apr 16, 2024 at 9:38 AM Anton Khirnov <anton@khirnov.net>
> > wrote:
> > 
> > > Quoting Tomas Härdin (2024-04-12 11:40:47)
> > > > This idea could be extended to other fields not presently
> > > > considered to
> > > > be metadata, that would be handy to treat as such.
> > > > 
> > > > I use the key "id" because ffprobe outputs id= for streamid.
> > > > Another
> > > > option could be to collect these types of metadata that go into
> > > > AVStream fields under a namespace like FFMPEG: or AVSTREAM: or
> > > > something, then delete all of them using AV_DICT_IGNORE_SUFFIX
> > > > near the
> > > > end of of_open() since they're for internal ffmpeg use.
> > > > 
> > > > The FATE change is just because av_dict() changes the order of
> > > > things
> > > > when elements are deleted.
> > > > 
> > > > /Tomas
> > > > 
> > > > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00
> > > > 2001
> > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > > > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > > > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> > > > 
> > > > This allows using -map_metadata and -metadata to copy/set
> > > > streamids
> > > (PIDs).
> > > 
> > > I dislike this patch, metadata is the wrong place for such
> > > information.
> 
> Seems like a matter of taste to me, but maybe I'm missing something

It's not just a matter of taste - it's happened several times already
that people (ab)used metadata to carry structured information and then
it turned out it was a bad idea and we had to change it to a real API.

Metadata really should only be used for unstructured user-facing
information like title/author/etc.. It's a terrible mechanism for other
usecases, because it's an implicit API hidden from the compiler, with no
type information, stability guarantees, deprecation mechanisms, etc. Not
to mention it forces users to parse and serialize strings, which is a
massive pain and a constant source of bugs, especially in C.

> In the very common case where users want to copy PIDs from inputs to
> outputs, implementing -map_streamid seems a bit silly. Consider also
> the case where the user wants to copy codec and bitrate from some
> source stream, such as when filtering audio. It would be nice if such
> operations were handled by a common mechanism. Call it -map_params
> perhaps

How long until ffmpeg CLI options are turing complete?
Tomas Härdin July 1, 2024, 2:58 p.m. UTC | #10 
mån 2024-07-01 klockan 16:51 +0200 skrev Anton Khirnov:
> Quoting Tomas Härdin (2024-04-16 16:12:13)
> > tis 2024-04-16 klockan 09:52 -0300 skrev James Almer:
> > > On Tue, Apr 16, 2024 at 9:38 AM Anton Khirnov <anton@khirnov.net>
> > > wrote:
> > > 
> > > > Quoting Tomas Härdin (2024-04-12 11:40:47)
> > > > > This idea could be extended to other fields not presently
> > > > > considered to
> > > > > be metadata, that would be handy to treat as such.
> > > > > 
> > > > > I use the key "id" because ffprobe outputs id= for streamid.
> > > > > Another
> > > > > option could be to collect these types of metadata that go
> > > > > into
> > > > > AVStream fields under a namespace like FFMPEG: or AVSTREAM:
> > > > > or
> > > > > something, then delete all of them using
> > > > > AV_DICT_IGNORE_SUFFIX
> > > > > near the
> > > > > end of of_open() since they're for internal ffmpeg use.
> > > > > 
> > > > > The FATE change is just because av_dict() changes the order
> > > > > of
> > > > > things
> > > > > when elements are deleted.
> > > > > 
> > > > > /Tomas
> > > > > 
> > > > > From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17
> > > > > 00:00:00
> > > > > 2001
> > > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> > > > > Date: Fri, 12 Apr 2024 10:34:12 +0200
> > > > > Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'
> > > > > 
> > > > > This allows using -map_metadata and -metadata to copy/set
> > > > > streamids
> > > > (PIDs).
> > > > 
> > > > I dislike this patch, metadata is the wrong place for such
> > > > information.
> > 
> > Seems like a matter of taste to me, but maybe I'm missing something
> 
> It's not just a matter of taste - it's happened several times already
> that people (ab)used metadata to carry structured information and
> then
> it turned out it was a bad idea and we had to change it to a real
> API.
> 
> Metadata really should only be used for unstructured user-facing
> information like title/author/etc.. It's a terrible mechanism for
> other
> usecases, because it's an implicit API hidden from the compiler, with
> no
> type information, stability guarantees, deprecation mechanisms, etc.
> Not
> to mention it forces users to parse and serialize strings, which is a
> massive pain and a constant source of bugs, especially in C.
> 
> > In the very common case where users want to copy PIDs from inputs
> > to
> > outputs, implementing -map_streamid seems a bit silly. Consider
> > also
> > the case where the user wants to copy codec and bitrate from some
> > source stream, such as when filtering audio. It would be nice if
> > such
> > operations were handled by a common mechanism. Call it -map_params
> > perhaps
> 
> How long until ffmpeg CLI options are turing complete?

*cough* lavu/eval.* *cough*

But yeah there's probably no way to make everyone happy with something
like this. In practice plenty of users maintain their own forks to
cover their own obscure use-cases

/Tomas
diff mbox series

Patch

From 7799f1b2eb8ab02e58118565f3e889fbe0d568a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Fri, 12 Apr 2024 10:34:12 +0200
Subject: [PATCH] ffmpeg: Carry streamid as metadata key 'id'

This allows using -map_metadata and -metadata to copy/set streamids (PIDs).
---
 fftools/ffmpeg_demux.c              |  5 +++
 fftools/ffmpeg_mux_init.c           | 56 +++++++++++++++++++++--------
 tests/ref/fate/matroska-stereo_mode |  6 ++--
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c
index cba63dab5f..1b0ef91abb 100644
--- a/fftools/ffmpeg_demux.c
+++ b/fftools/ffmpeg_demux.c
@@ -1454,6 +1454,11 @@  static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
 
     ds->codec_desc = avcodec_descriptor_get(ist->par->codec_id);
 
+    // carry streamid as metadata so that streamids can be be handled like any other metadata
+    // be careful not to overwrite any existing value, just in case
+    if ((ret = av_dict_set_int(&st->metadata, "id", st->id, AV_DICT_DONT_OVERWRITE)) < 0)
+        av_log(ist, AV_LOG_ERROR, "error %i setting \"id\" to %i. id already set?\n", ret, st->id);
+
     return 0;
 }
 
diff --git a/fftools/ffmpeg_mux_init.c b/fftools/ffmpeg_mux_init.c
index 6d8bd5bcdf..dbe16e8d0f 100644
--- a/fftools/ffmpeg_mux_init.c
+++ b/fftools/ffmpeg_mux_init.c
@@ -1073,21 +1073,6 @@  static int ost_add(Muxer *mux, const OptionsContext *o, enum AVMediaType type,
 
     ost = &ms->ost;
 
-    if (o->streamid) {
-        AVDictionaryEntry *e;
-        char idx[16], *p;
-        snprintf(idx, sizeof(idx), "%d", ost->index);
-
-        e = av_dict_get(o->streamid, idx, NULL, 0);
-        if (e) {
-            st->id = strtol(e->value, &p, 0);
-            if (!e->value[0] || *p) {
-                av_log(ost, AV_LOG_FATAL, "Invalid stream id: %s\n", e->value);
-                return AVERROR(EINVAL);
-            }
-        }
-    }
-
     ost->par_in = avcodec_parameters_alloc();
     if (!ost->par_in)
         return AVERROR(ENOMEM);
@@ -3016,6 +3001,43 @@  static Muxer *mux_alloc(void)
     return mux;
 }
 
+static int of_set_streamid(Muxer *mux, const OptionsContext *o)
+{
+    for (int i = 0; i < mux->fc->nb_streams; i++) {
+        OutputStream *ost = mux->of.streams[i];
+        AVDictionaryEntry *e = NULL;
+
+        // take -streamid if explicitly set
+        if (o->streamid) {
+            char idx[16];
+            snprintf(idx, sizeof(idx), "%d", ost->index);
+
+            e = av_dict_get(o->streamid, idx, NULL, 0);
+        }
+
+        // if -streamid not set then try to grab it from metadata
+        // this maintains backward compatibility
+        if (!e)
+            e = av_dict_get(ost->st->metadata, "id", NULL, 0);
+
+        if (e) {
+            char *p;
+            ost->st->id = strtol(e->value, &p, 0);
+            if (!e->value[0] || *p) {
+                av_log(ost, AV_LOG_FATAL, "Invalid stream id: %s\n", e->value);
+                return AVERROR(EINVAL);
+            } else {
+                // delete id now that we've picked it up,
+                // so that it doesn't make it into the output
+                int ret = av_dict_set(&ost->st->metadata, "id", NULL, 0);
+                if (ret < 0)
+                    return ret;
+            }
+        }
+    }
+    return 0;
+}
+
 int of_open(const OptionsContext *o, const char *filename, Scheduler *sch)
 {
     Muxer *mux;
@@ -3149,6 +3171,10 @@  int of_open(const OptionsContext *o, const char *filename, Scheduler *sch)
     if (err < 0)
         return err;
 
+    err = of_set_streamid(mux, o);
+    if (err < 0)
+        return err;
+
     err = set_dispositions(mux, o);
     if (err < 0) {
         av_log(mux, AV_LOG_FATAL, "Error setting output stream dispositions\n");
diff --git a/tests/ref/fate/matroska-stereo_mode b/tests/ref/fate/matroska-stereo_mode
index 739b789fea..e46f75cbb4 100644
--- a/tests/ref/fate/matroska-stereo_mode
+++ b/tests/ref/fate/matroska-stereo_mode
@@ -1,4 +1,4 @@ 
-a7a220a77001e81685ec807ce5ac3bc6 *tests/data/fate/matroska-stereo_mode.matroska
+40d2771cf74d378476cc4764b87af156 *tests/data/fate/matroska-stereo_mode.matroska
 1470764 tests/data/fate/matroska-stereo_mode.matroska
 #extradata 0:     3510, 0x560c3919
 #extradata 1:     3510, 0x560c3919
@@ -125,8 +125,8 @@  DISPOSITION:dub=0
 DISPOSITION:original=0
 TAG:language=ger-at
 TAG:stereo_mode=left_right
-TAG:DESCRIPTION-ger=Deutsch
 TAG:DESCRIPTION-fre=Francais
+TAG:DESCRIPTION-ger=Deutsch
 TAG:DURATION=00:00:10.000000000
 [SIDE_DATA]
 side_data_type=Stereo 3D
@@ -140,8 +140,8 @@  DISPOSITION:dub=0
 DISPOSITION:original=0
 TAG:language=eng
 TAG:stereo_mode=bottom_top
-TAG:DESCRIPTION-ger=Deutsch
 TAG:DESCRIPTION-fre=Francais
+TAG:DESCRIPTION-ger=Deutsch
 TAG:DURATION=00:00:10.000000000
 [SIDE_DATA]
 side_data_type=Stereo 3D
-- 
2.39.2