[FFmpeg-devel,v4,1/3] avformat: add fields to AVProgram/AVStream for PMT change tracking

Submitted by Aman Gupta on May 18, 2018, 6:15 p.m.

Details

Message ID 20180518181506.88903-1-ffmpeg@tmm1.net
State Accepted
Commit 2b2f2f65f38cdd64fe126079f84872c0b06c6afc
Headers show

Commit Message

Aman Gupta May 18, 2018, 6:15 p.m.
From: Aman Gupta <aman@tmm1.net>

These fields will allow the mpegts demuxer to expose details about
the PMT/program which created the AVProgram and its AVStreams.

In mpegts, a PMT which advertises streams has a version number
which can be incremented at any time. When the version changes,
the pids which correspond to each of it's streams can also change.

Since ffmpeg creates a new AVStream per pid by default, an API user
needs the ability to (a) detect when the PMT changed, and (b) tell
which AVStream were added to replace earlier streams.

This has been a long-standing issue with ffmpeg's handling of mpegts
streams with PMT changes, and I found two related patches in the wild
that attempt to solve the same problem.

The first is in MythTV's ffmpeg fork, where they added a
void (*streams_changed)(void*); to AVFormatContext and call it from
their fork of the mpegts demuxer whenever the PMT changes.

The second was proposed by XBMC in
https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
attempted to send packets to it whenever the PMT changed.

Signed-off-by: Aman Gupta <aman@tmm1.net>
---
 doc/APIchanges         | 4 ++++
 libavformat/avformat.h | 8 ++++++++
 libavformat/utils.c    | 1 +
 libavformat/version.h  | 2 +-
 4 files changed, 14 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer May 19, 2018, 9:31 p.m.
On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> These fields will allow the mpegts demuxer to expose details about
> the PMT/program which created the AVProgram and its AVStreams.
> 
> In mpegts, a PMT which advertises streams has a version number
> which can be incremented at any time. When the version changes,
> the pids which correspond to each of it's streams can also change.
> 
> Since ffmpeg creates a new AVStream per pid by default, an API user
> needs the ability to (a) detect when the PMT changed, and (b) tell
> which AVStream were added to replace earlier streams.
> 
> This has been a long-standing issue with ffmpeg's handling of mpegts
> streams with PMT changes, and I found two related patches in the wild
> that attempt to solve the same problem.
> 
> The first is in MythTV's ffmpeg fork, where they added a
> void (*streams_changed)(void*); to AVFormatContext and call it from
> their fork of the mpegts demuxer whenever the PMT changes.
> 
> The second was proposed by XBMC in
> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
> attempted to send packets to it whenever the PMT changed.
> 
> Signed-off-by: Aman Gupta <aman@tmm1.net>
> ---
>  doc/APIchanges         | 4 ++++
>  libavformat/avformat.h | 8 ++++++++
>  libavformat/utils.c    | 1 +
>  libavformat/version.h  | 2 +-
>  4 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index befa58c84a..a592073ca5 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
> +  Add pmt_version field to AVProgram
> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
> +
>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
>    Add AV_DISPOSITION_STILL_IMAGE
>  
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 6dce88fad5..ade918f99c 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
>       */
>      int stream_identifier;
>  
> +    /**
> +     * Details of the MPEG-TS program which created this stream.
> +     */
> +    int program_num;
> +    int pmt_version;
> +    int pmt_stream_idx;
> +
>      int64_t interleaver_chunk_size;
>      int64_t interleaver_chunk_duration;
>  

These are added below the "All fields below this line are not part of the public API."
This contradicts the addition to APIChanges

[...]
James Almer May 19, 2018, 9:48 p.m.
On 5/19/2018 6:31 PM, Michael Niedermayer wrote:
> On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
>> From: Aman Gupta <aman@tmm1.net>
>>
>> These fields will allow the mpegts demuxer to expose details about
>> the PMT/program which created the AVProgram and its AVStreams.
>>
>> In mpegts, a PMT which advertises streams has a version number
>> which can be incremented at any time. When the version changes,
>> the pids which correspond to each of it's streams can also change.
>>
>> Since ffmpeg creates a new AVStream per pid by default, an API user
>> needs the ability to (a) detect when the PMT changed, and (b) tell
>> which AVStream were added to replace earlier streams.
>>
>> This has been a long-standing issue with ffmpeg's handling of mpegts
>> streams with PMT changes, and I found two related patches in the wild
>> that attempt to solve the same problem.
>>
>> The first is in MythTV's ffmpeg fork, where they added a
>> void (*streams_changed)(void*); to AVFormatContext and call it from
>> their fork of the mpegts demuxer whenever the PMT changes.
>>
>> The second was proposed by XBMC in
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
>> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
>> attempted to send packets to it whenever the PMT changed.
>>
>> Signed-off-by: Aman Gupta <aman@tmm1.net>
>> ---
>>  doc/APIchanges         | 4 ++++
>>  libavformat/avformat.h | 8 ++++++++
>>  libavformat/utils.c    | 1 +
>>  libavformat/version.h  | 2 +-
>>  4 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index befa58c84a..a592073ca5 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>  
>>  API changes, most recent first:
>>  
>> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
>> +  Add pmt_version field to AVProgram
>> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
>> +
>>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
>>    Add AV_DISPOSITION_STILL_IMAGE
>>  
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 6dce88fad5..ade918f99c 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
>>       */
>>      int stream_identifier;
>>  
>> +    /**
>> +     * Details of the MPEG-TS program which created this stream.
>> +     */
>> +    int program_num;
>> +    int pmt_version;
>> +    int pmt_stream_idx;
>> +
>>      int64_t interleaver_chunk_size;
>>      int64_t interleaver_chunk_duration;
>>  
> 
> These are added below the "All fields below this line are not part of the public API."
> This contradicts the addition to APIChanges

If these don't need to be accessed by the API user then I'd rather keep
them here than moving them to the public section. Just remove the
APIChanges entry.
Same thing with the AVStream pmt_version field. If direct access is not
needed for it, then it should be moved below the public notice.

These fields seem pretty specific to one demuxer so ideally they
shouldn't be public in case changes to them and the implementation are
ever needed.
Carl Eugen Hoyos May 20, 2018, 10:54 a.m.
2018-05-19 23:48 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 5/19/2018 6:31 PM, Michael Niedermayer wrote:
>> On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
>>> From: Aman Gupta <aman@tmm1.net>
>>>
>>> These fields will allow the mpegts demuxer to expose details about
>>> the PMT/program which created the AVProgram and its AVStreams.
>>>
>>> In mpegts, a PMT which advertises streams has a version number
>>> which can be incremented at any time. When the version changes,
>>> the pids which correspond to each of it's streams can also change.
>>>
>>> Since ffmpeg creates a new AVStream per pid by default, an API user
>>> needs the ability to (a) detect when the PMT changed, and (b) tell
>>> which AVStream were added to replace earlier streams.
>>>
>>> This has been a long-standing issue with ffmpeg's handling of mpegts
>>> streams with PMT changes, and I found two related patches in the wild
>>> that attempt to solve the same problem.
>>>
>>> The first is in MythTV's ffmpeg fork, where they added a
>>> void (*streams_changed)(void*); to AVFormatContext and call it from
>>> their fork of the mpegts demuxer whenever the PMT changes.
>>>
>>> The second was proposed by XBMC in
>>> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
>>> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
>>> attempted to send packets to it whenever the PMT changed.
>>>
>>> Signed-off-by: Aman Gupta <aman@tmm1.net>
>>> ---
>>>  doc/APIchanges         | 4 ++++
>>>  libavformat/avformat.h | 8 ++++++++
>>>  libavformat/utils.c    | 1 +
>>>  libavformat/version.h  | 2 +-
>>>  4 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index befa58c84a..a592073ca5 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>>
>>>  API changes, most recent first:
>>>
>>> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
>>> +  Add pmt_version field to AVProgram
>>> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
>>> +
>>>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
>>>    Add AV_DISPOSITION_STILL_IMAGE
>>>
>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>> index 6dce88fad5..ade918f99c 100644
>>> --- a/libavformat/avformat.h
>>> +++ b/libavformat/avformat.h
>>> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
>>>       */
>>>      int stream_identifier;
>>>
>>> +    /**
>>> +     * Details of the MPEG-TS program which created this stream.
>>> +     */
>>> +    int program_num;
>>> +    int pmt_version;
>>> +    int pmt_stream_idx;
>>> +
>>>      int64_t interleaver_chunk_size;
>>>      int64_t interleaver_chunk_duration;
>>>
>>
>> These are added below the "All fields below this line are not part of the
>> public API."
>> This contradicts the addition to APIChanges
>
> If these don't need to be accessed by the API user then I'd rather keep
> them here than moving them to the public section. Just remove the
> APIChanges entry.
> Same thing with the AVStream pmt_version field. If direct access is not
> needed for it, then it should be moved below the public notice.
>
> These fields seem pretty specific to one demuxer so ideally they
> shouldn't be public in case changes to them and the implementation are
> ever needed.

Reading the original submission above, isn't the whole point of the
patch to add public symbols to help downstream with real-world
issues?

Carl Eugen
Jan Ekström May 20, 2018, 11:06 a.m.
On Sun, May 20, 2018 at 1:54 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Reading the original submission above, isn't the whole point of the
> patch to add public symbols to help downstream with real-world
> issues?
>
> Carl Eugen

Yes, currently the API user has no visibility over how the programs
are being updated. Two alternative takes on this were listed in the
commit message, one being a call-back and another being a custom data
packet from stream id 0.

So yes, the pmt_version is most definitely meant to be publicly
utilized, with it the proper utilization would go as follows:
1) API client picks the program(s) it cares about, and gets the
AVProgram(s)' pointers and notes the pmt_version when it started
reading.
2) After each AVPacket read, it checks if the pmt_version was updated.
If it was, it iterates over the streams currently registered in the
AVProgram(s) it cares about.
3) If any streams got dropped, those should be no longer cared about
by the API client. If any streams got added, they should be checked by
the API client if it is interested in them.

This is an upgrade on the previous state of affairs which would have
required the following:
1) API client picks the program(s) it cares about, and gets the
AVProgram(s)' pointers.
2) After each AVPacket read, it iterates over the streams currently
registered in the AVProgram(s) it cares about.
3) If any streams got dropped, those should be no longer cared about
by the API client. If any streams got added, they should be checked by
the API client if it is interested in them.

It is not perfect, but still an upgrade, since you can just check an
integer now, instead of doing the full iteration every time you read
an AVPacket.


Best regards,
Jan
Jan Ekström May 20, 2018, 2:21 p.m.
On Sun, May 20, 2018 at 2:06 PM, Jan Ekström <jeebjp@gmail.com> wrote:
> On Sun, May 20, 2018 at 1:54 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>> Reading the original submission above, isn't the whole point of the
>> patch to add public symbols to help downstream with real-world
>> issues?
>>
>> Carl Eugen
>
> Yes, currently the API user has no visibility over how the programs
> are being updated. Two alternative takes on this were listed in the
> commit message, one being a call-back and another being a custom data
> packet from stream id 0.
>
> So yes, the pmt_version is most definitely meant to be publicly
> utilized, with it the proper utilization would go as follows:
> 1) API client picks the program(s) it cares about, and gets the
> AVProgram(s)' pointers and notes the pmt_version when it started
> reading.
> 2) After each AVPacket read, it checks if the pmt_version was updated.
> If it was, it iterates over the streams currently registered in the
> AVProgram(s) it cares about.
> 3) If any streams got dropped, those should be no longer cared about
> by the API client. If any streams got added, they should be checked by
> the API client if it is interested in them.
>
> This is an upgrade on the previous state of affairs which would have
> required the following:
> 1) API client picks the program(s) it cares about, and gets the
> AVProgram(s)' pointers.
> 2) After each AVPacket read, it iterates over the streams currently
> registered in the AVProgram(s) it cares about.
> 3) If any streams got dropped, those should be no longer cared about
> by the API client. If any streams got added, they should be checked by
> the API client if it is interested in them.
>
> It is not perfect, but still an upgrade, since you can just check an
> integer now, instead of doing the full iteration every time you read
> an AVPacket.
>
>
> Best regards,
> Jan

Oh, and I'm not sure if lavf's internal reordering of packets could
make it happen that you receive the program update, and then still
receive one or more packets from the previous stream. In that case you
might also incorporate some check that the flush and reset of previous
decoder only happens after you get your first new packet for that new
stream.

Jan
Aman Gupta May 20, 2018, 6:37 p.m.
On Sat, May 19, 2018 at 2:56 PM James Almer <jamrial@gmail.com> wrote:

> On 5/19/2018 6:31 PM, Michael Niedermayer wrote:
> > On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
> >> From: Aman Gupta <aman@tmm1.net>
> >>
> >> These fields will allow the mpegts demuxer to expose details about
> >> the PMT/program which created the AVProgram and its AVStreams.
> >>
> >> In mpegts, a PMT which advertises streams has a version number
> >> which can be incremented at any time. When the version changes,
> >> the pids which correspond to each of it's streams can also change.
> >>
> >> Since ffmpeg creates a new AVStream per pid by default, an API user
> >> needs the ability to (a) detect when the PMT changed, and (b) tell
> >> which AVStream were added to replace earlier streams.
> >>
> >> This has been a long-standing issue with ffmpeg's handling of mpegts
> >> streams with PMT changes, and I found two related patches in the wild
> >> that attempt to solve the same problem.
> >>
> >> The first is in MythTV's ffmpeg fork, where they added a
> >> void (*streams_changed)(void*); to AVFormatContext and call it from
> >> their fork of the mpegts demuxer whenever the PMT changes.
> >>
> >> The second was proposed by XBMC in
> >> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
> >> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
> >> attempted to send packets to it whenever the PMT changed.
> >>
> >> Signed-off-by: Aman Gupta <aman@tmm1.net>
> >> ---
> >>  doc/APIchanges         | 4 ++++
> >>  libavformat/avformat.h | 8 ++++++++
> >>  libavformat/utils.c    | 1 +
> >>  libavformat/version.h  | 2 +-
> >>  4 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index befa58c84a..a592073ca5 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
> >>
> >>  API changes, most recent first:
> >>
> >> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
> >> +  Add pmt_version field to AVProgram
> >> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
> >> +
> >>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
> >>    Add AV_DISPOSITION_STILL_IMAGE
> >>
> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >> index 6dce88fad5..ade918f99c 100644
> >> --- a/libavformat/avformat.h
> >> +++ b/libavformat/avformat.h
> >> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
> >>       */
> >>      int stream_identifier;
> >>
> >> +    /**
> >> +     * Details of the MPEG-TS program which created this stream.
> >> +     */
> >> +    int program_num;
> >> +    int pmt_version;
> >> +    int pmt_stream_idx;
> >> +
> >>      int64_t interleaver_chunk_size;
> >>      int64_t interleaver_chunk_duration;
> >>
> >
> > These are added below the "All fields below this line are not part of
> the public API."
> > This contradicts the addition to APIChanges
>
> If these don't need to be accessed by the API user then I'd rather keep
> them here than moving them to the public section. Just remove the
> APIChanges entry.
> Same thing with the AVStream pmt_version field. If direct access is not
> needed for it, then it should be moved below the public notice.
>
> These fields seem pretty specific to one demuxer so ideally they
> shouldn't be public in case changes to them and the implementation are
> ever needed.


Yes they are specific to mpegts, and also may need to change in the future.

These were intended to be used in conjunction with the existing
AVStream.stream_identifier, which is also mpegts-specific. It appears in
the private section, even though the field is only written to from mpegts.c
and never used anywhere else in avformat. Clearly it was added to be used
by API users, but it too lives in the private section.

It seems like the best course forward is to remove APIchanges entries. The
fields are there if a user really needs to access them, but they are
considered private which means we can change them later if need be to
evolve support. In this case, should I revert the version bump as well? Or
is that still required since the ABI changed (even though it was in the
private section).

Aman


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
James Almer May 20, 2018, 7:10 p.m.
On 5/20/2018 3:37 PM, Aman Gupta wrote:
> On Sat, May 19, 2018 at 2:56 PM James Almer <jamrial@gmail.com> wrote:
> 
>> On 5/19/2018 6:31 PM, Michael Niedermayer wrote:
>>> On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
>>>> From: Aman Gupta <aman@tmm1.net>
>>>>
>>>> These fields will allow the mpegts demuxer to expose details about
>>>> the PMT/program which created the AVProgram and its AVStreams.
>>>>
>>>> In mpegts, a PMT which advertises streams has a version number
>>>> which can be incremented at any time. When the version changes,
>>>> the pids which correspond to each of it's streams can also change.
>>>>
>>>> Since ffmpeg creates a new AVStream per pid by default, an API user
>>>> needs the ability to (a) detect when the PMT changed, and (b) tell
>>>> which AVStream were added to replace earlier streams.
>>>>
>>>> This has been a long-standing issue with ffmpeg's handling of mpegts
>>>> streams with PMT changes, and I found two related patches in the wild
>>>> that attempt to solve the same problem.
>>>>
>>>> The first is in MythTV's ffmpeg fork, where they added a
>>>> void (*streams_changed)(void*); to AVFormatContext and call it from
>>>> their fork of the mpegts demuxer whenever the PMT changes.
>>>>
>>>> The second was proposed by XBMC in
>>>> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
>>>> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
>>>> attempted to send packets to it whenever the PMT changed.
>>>>
>>>> Signed-off-by: Aman Gupta <aman@tmm1.net>
>>>> ---
>>>>  doc/APIchanges         | 4 ++++
>>>>  libavformat/avformat.h | 8 ++++++++
>>>>  libavformat/utils.c    | 1 +
>>>>  libavformat/version.h  | 2 +-
>>>>  4 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>> index befa58c84a..a592073ca5 100644
>>>> --- a/doc/APIchanges
>>>> +++ b/doc/APIchanges
>>>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>>>
>>>>  API changes, most recent first:
>>>>
>>>> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
>>>> +  Add pmt_version field to AVProgram
>>>> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
>>>> +
>>>>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
>>>>    Add AV_DISPOSITION_STILL_IMAGE
>>>>
>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>>> index 6dce88fad5..ade918f99c 100644
>>>> --- a/libavformat/avformat.h
>>>> +++ b/libavformat/avformat.h
>>>> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
>>>>       */
>>>>      int stream_identifier;
>>>>
>>>> +    /**
>>>> +     * Details of the MPEG-TS program which created this stream.
>>>> +     */
>>>> +    int program_num;
>>>> +    int pmt_version;
>>>> +    int pmt_stream_idx;
>>>> +
>>>>      int64_t interleaver_chunk_size;
>>>>      int64_t interleaver_chunk_duration;
>>>>
>>>
>>> These are added below the "All fields below this line are not part of
>> the public API."
>>> This contradicts the addition to APIChanges
>>
>> If these don't need to be accessed by the API user then I'd rather keep
>> them here than moving them to the public section. Just remove the
>> APIChanges entry.
>> Same thing with the AVStream pmt_version field. If direct access is not
>> needed for it, then it should be moved below the public notice.
>>
>> These fields seem pretty specific to one demuxer so ideally they
>> shouldn't be public in case changes to them and the implementation are
>> ever needed.
> 
> 
> Yes they are specific to mpegts, and also may need to change in the future.
> 
> These were intended to be used in conjunction with the existing
> AVStream.stream_identifier, which is also mpegts-specific. It appears in
> the private section, even though the field is only written to from mpegts.c
> and never used anywhere else in avformat. Clearly it was added to be used
> by API users, but it too lives in the private section.
> 
> It seems like the best course forward is to remove APIchanges entries. The
> fields are there if a user really needs to access them, but they are
> considered private which means we can change them later if need be to
> evolve support. In this case, should I revert the version bump as well? Or
> is that still required since the ABI changed (even though it was in the
> private section).

Since they are in the private section then yes, remove the relevant
APIChanges entry, but don't revert the version bump (It's never a clean
thing to do and it's best to avoid it).
For that matter, new fields added to the private section of these
structs don't change the ABI since their size isn't part of it. They can
be removed and changed at any time without a major bump, which is why
API users are not meant to ever access them directly.

And again, if AVStream.pmt_version is also meant to be private and
change in the future, then please move it to the private section.
Michael Niedermayer May 20, 2018, 7:55 p.m.
On Sun, May 20, 2018 at 11:37:22AM -0700, Aman Gupta wrote:
> On Sat, May 19, 2018 at 2:56 PM James Almer <jamrial@gmail.com> wrote:
> 
> > On 5/19/2018 6:31 PM, Michael Niedermayer wrote:
> > > On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
> > >> From: Aman Gupta <aman@tmm1.net>
> > >>
> > >> These fields will allow the mpegts demuxer to expose details about
> > >> the PMT/program which created the AVProgram and its AVStreams.
> > >>
> > >> In mpegts, a PMT which advertises streams has a version number
> > >> which can be incremented at any time. When the version changes,
> > >> the pids which correspond to each of it's streams can also change.
> > >>
> > >> Since ffmpeg creates a new AVStream per pid by default, an API user
> > >> needs the ability to (a) detect when the PMT changed, and (b) tell
> > >> which AVStream were added to replace earlier streams.
> > >>
> > >> This has been a long-standing issue with ffmpeg's handling of mpegts
> > >> streams with PMT changes, and I found two related patches in the wild
> > >> that attempt to solve the same problem.
> > >>
> > >> The first is in MythTV's ffmpeg fork, where they added a
> > >> void (*streams_changed)(void*); to AVFormatContext and call it from
> > >> their fork of the mpegts demuxer whenever the PMT changes.
> > >>
> > >> The second was proposed by XBMC in
> > >> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html,
> > >> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
> > >> attempted to send packets to it whenever the PMT changed.
> > >>
> > >> Signed-off-by: Aman Gupta <aman@tmm1.net>
> > >> ---
> > >>  doc/APIchanges         | 4 ++++
> > >>  libavformat/avformat.h | 8 ++++++++
> > >>  libavformat/utils.c    | 1 +
> > >>  libavformat/version.h  | 2 +-
> > >>  4 files changed, 14 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/doc/APIchanges b/doc/APIchanges
> > >> index befa58c84a..a592073ca5 100644
> > >> --- a/doc/APIchanges
> > >> +++ b/doc/APIchanges
> > >> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
> > >>
> > >>  API changes, most recent first:
> > >>
> > >> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
> > >> +  Add pmt_version field to AVProgram
> > >> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
> > >> +
> > >>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
> > >>    Add AV_DISPOSITION_STILL_IMAGE
> > >>
> > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > >> index 6dce88fad5..ade918f99c 100644
> > >> --- a/libavformat/avformat.h
> > >> +++ b/libavformat/avformat.h
> > >> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
> > >>       */
> > >>      int stream_identifier;
> > >>
> > >> +    /**
> > >> +     * Details of the MPEG-TS program which created this stream.
> > >> +     */
> > >> +    int program_num;
> > >> +    int pmt_version;
> > >> +    int pmt_stream_idx;
> > >> +
> > >>      int64_t interleaver_chunk_size;
> > >>      int64_t interleaver_chunk_duration;
> > >>
> > >
> > > These are added below the "All fields below this line are not part of
> > the public API."
> > > This contradicts the addition to APIChanges
> >
> > If these don't need to be accessed by the API user then I'd rather keep
> > them here than moving them to the public section. Just remove the
> > APIChanges entry.
> > Same thing with the AVStream pmt_version field. If direct access is not
> > needed for it, then it should be moved below the public notice.
> >
> > These fields seem pretty specific to one demuxer so ideally they
> > shouldn't be public in case changes to them and the implementation are
> > ever needed.
> 
> 
> Yes they are specific to mpegts, and also may need to change in the future.
> 
> These were intended to be used in conjunction with the existing
> AVStream.stream_identifier, which is also mpegts-specific. It appears in
> the private section, even though the field is only written to from mpegts.c
> and never used anywhere else in avformat. Clearly it was added to be used
> by API users, but it too lives in the private section.
> 

> It seems like the best course forward is to remove APIchanges entries. The
> fields are there if a user really needs to access them, but they are
> considered private which means we can change them later if need be to
> evolve support. In this case, should I revert the version bump as well? Or
> is that still required since the ABI changed (even though it was in the
> private section).

Iam not sure i understand what you suggest but if a field is added in the
private section then a user app cannot reliably access it. (It would move
in memory the next time a public field is added)

Also if the ABI changes in the future then any users using the API could break
(depending on the exact change)

[...]
Aman Gupta May 21, 2018, 8:10 p.m.
On Sun, May 20, 2018 at 12:55 PM, Michael Niedermayer <
michael@niedermayer.cc> wrote:

> On Sun, May 20, 2018 at 11:37:22AM -0700, Aman Gupta wrote:
> > On Sat, May 19, 2018 at 2:56 PM James Almer <jamrial@gmail.com> wrote:
> >
> > > On 5/19/2018 6:31 PM, Michael Niedermayer wrote:
> > > > On Fri, May 18, 2018 at 11:15:04AM -0700, Aman Gupta wrote:
> > > >> From: Aman Gupta <aman@tmm1.net>
> > > >>
> > > >> These fields will allow the mpegts demuxer to expose details about
> > > >> the PMT/program which created the AVProgram and its AVStreams.
> > > >>
> > > >> In mpegts, a PMT which advertises streams has a version number
> > > >> which can be incremented at any time. When the version changes,
> > > >> the pids which correspond to each of it's streams can also change.
> > > >>
> > > >> Since ffmpeg creates a new AVStream per pid by default, an API user
> > > >> needs the ability to (a) detect when the PMT changed, and (b) tell
> > > >> which AVStream were added to replace earlier streams.
> > > >>
> > > >> This has been a long-standing issue with ffmpeg's handling of mpegts
> > > >> streams with PMT changes, and I found two related patches in the
> wild
> > > >> that attempt to solve the same problem.
> > > >>
> > > >> The first is in MythTV's ffmpeg fork, where they added a
> > > >> void (*streams_changed)(void*); to AVFormatContext and call it from
> > > >> their fork of the mpegts demuxer whenever the PMT changes.
> > > >>
> > > >> The second was proposed by XBMC in
> > > >> https://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135036.html
> ,
> > > >> where they created a new AVMEDIA_TYPE_DATA stream with id=0 and
> > > >> attempted to send packets to it whenever the PMT changed.
> > > >>
> > > >> Signed-off-by: Aman Gupta <aman@tmm1.net>
> > > >> ---
> > > >>  doc/APIchanges         | 4 ++++
> > > >>  libavformat/avformat.h | 8 ++++++++
> > > >>  libavformat/utils.c    | 1 +
> > > >>  libavformat/version.h  | 2 +-
> > > >>  4 files changed, 14 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/doc/APIchanges b/doc/APIchanges
> > > >> index befa58c84a..a592073ca5 100644
> > > >> --- a/doc/APIchanges
> > > >> +++ b/doc/APIchanges
> > > >> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
> > > >>
> > > >>  API changes, most recent first:
> > > >>
> > > >> +2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
> > > >> +  Add pmt_version field to AVProgram
> > > >> +  Add program_num, pmt_version, pmt_stream_idx to AVStream
> > > >> +
> > > >>  2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
> > > >>    Add AV_DISPOSITION_STILL_IMAGE
> > > >>
> > > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > > >> index 6dce88fad5..ade918f99c 100644
> > > >> --- a/libavformat/avformat.h
> > > >> +++ b/libavformat/avformat.h
> > > >> @@ -1103,6 +1103,13 @@ typedef struct AVStream {
> > > >>       */
> > > >>      int stream_identifier;
> > > >>
> > > >> +    /**
> > > >> +     * Details of the MPEG-TS program which created this stream.
> > > >> +     */
> > > >> +    int program_num;
> > > >> +    int pmt_version;
> > > >> +    int pmt_stream_idx;
> > > >> +
> > > >>      int64_t interleaver_chunk_size;
> > > >>      int64_t interleaver_chunk_duration;
> > > >>
> > > >
> > > > These are added below the "All fields below this line are not part of
> > > the public API."
> > > > This contradicts the addition to APIChanges
> > >
> > > If these don't need to be accessed by the API user then I'd rather keep
> > > them here than moving them to the public section. Just remove the
> > > APIChanges entry.
> > > Same thing with the AVStream pmt_version field. If direct access is not
> > > needed for it, then it should be moved below the public notice.
> > >
> > > These fields seem pretty specific to one demuxer so ideally they
> > > shouldn't be public in case changes to them and the implementation are
> > > ever needed.
> >
> >
> > Yes they are specific to mpegts, and also may need to change in the
> future.
> >
> > These were intended to be used in conjunction with the existing
> > AVStream.stream_identifier, which is also mpegts-specific. It appears in
> > the private section, even though the field is only written to from
> mpegts.c
> > and never used anywhere else in avformat. Clearly it was added to be used
> > by API users, but it too lives in the private section.
> >
>
> > It seems like the best course forward is to remove APIchanges entries.
> The
> > fields are there if a user really needs to access them, but they are
> > considered private which means we can change them later if need be to
> > evolve support. In this case, should I revert the version bump as well?
> Or
> > is that still required since the ABI changed (even though it was in the
> > private section).
>
> Iam not sure i understand what you suggest but if a field is added in the
> private section then a user app cannot reliably access it. (It would move
> in memory the next time a public field is added)
>
> Also if the ABI changes in the future then any users using the API could
> break
> (depending on the exact change)
>

I understand. If it's in the private section, it's not safe to use
externally. If you do try to use it anyway, you need to take the proper
precautions.

I went ahead and removed the AVStream fields from the APIchanges files. I
would like to make these fields public eventually, but I will wait a while
for them to stabilize.

The new field on AVProgram is considerably more useful for public use, and
I have left it as a public field with the existing note in APIchanges.

Aman


>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>

Patch hide | download patch | download mbox

diff --git a/doc/APIchanges b/doc/APIchanges
index befa58c84a..a592073ca5 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,10 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2018-05-xx - xxxxxxxxxx - lavf 58.15.100 - avformat.h
+  Add pmt_version field to AVProgram
+  Add program_num, pmt_version, pmt_stream_idx to AVStream
+
 2018-05-xx - xxxxxxxxxx - lavf 58.14.100 - avformat.h
   Add AV_DISPOSITION_STILL_IMAGE
 
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 6dce88fad5..ade918f99c 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -1103,6 +1103,13 @@  typedef struct AVStream {
      */
     int stream_identifier;
 
+    /**
+     * Details of the MPEG-TS program which created this stream.
+     */
+    int program_num;
+    int pmt_version;
+    int pmt_stream_idx;
+
     int64_t interleaver_chunk_size;
     int64_t interleaver_chunk_duration;
 
@@ -1260,6 +1267,7 @@  typedef struct AVProgram {
     int program_num;
     int pmt_pid;
     int pcr_pid;
+    int pmt_version;
 
     /*****************************************************************
      * All fields below this line are not part of the public API. They
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 636fae3779..a4aa4e10b1 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4569,6 +4569,7 @@  AVProgram *av_new_program(AVFormatContext *ac, int id)
             return NULL;
         dynarray_add(&ac->programs, &ac->nb_programs, program);
         program->discard = AVDISCARD_NONE;
+        program->pmt_version = -1;
     }
     program->id = id;
     program->pts_wrap_reference = AV_NOPTS_VALUE;
diff --git a/libavformat/version.h b/libavformat/version.h
index e9b94cc216..c8e89cdce1 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,7 +32,7 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  58
-#define LIBAVFORMAT_VERSION_MINOR  14
+#define LIBAVFORMAT_VERSION_MINOR  15
 #define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \