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

Aman Gupta ffmpeg at tmm1.net
Mon May 21 23:10:41 EEST 2018


On Sun, May 20, 2018 at 12:55 PM, Michael Niedermayer <
michael at 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 at 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 at 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 at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>


More information about the ffmpeg-devel mailing list