On Sun, May 20, 2018 at 12:55 PM, Michael Niedermayer < mich...@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 <jamr...@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 <a...@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 <a...@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 > > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
