| Message ID | 20201009130430.602-18-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/18] lavf: move AVStream.info to AVStreamInternal | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Fri, Oct 09, 2020 at 03:04:30PM +0200, Anton Khirnov wrote: > Specifically: pts_wrap_bits, first_dts, cur_dts. > They are supposed to be private and are located in the private section > of AVStream, but ffmpeg.c currently accesses them regardless. They > should be moved to AVStreamInternal once that bug is fixed. > > Remove the marker for the private AVStream section, as there are no > other accessible fields left there. It has proven highly confusing and > people kept adding supposedly-public fields into the private section. > New private per-stream fields should be added to AVStreamInternal. > --- > libavformat/avformat.h | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index a01912d654..612791a2eb 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -1013,22 +1013,16 @@ typedef struct AVStream { > */ > AVCodecParameters *codecpar; > > - /***************************************************************** > - * All fields below this line are not part of the public API. They > - * may not be used outside of libavformat and can be changed and > - * removed at will. > - * Internal note: be aware that physically removing these fields > - * will break ABI. Replace removed fields with dummy fields, and > - * add new fields to AVStreamInternal. > - ***************************************************************** > - */ > - > #if LIBAVFORMAT_VERSION_MAJOR < 59 > // kept for ABI compatibility only, do not access in any way > void *unused; > #endif > > - int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */ > + /** > + * number of bits in pts (used for wrapping control) > + * private, do not access from outside libavformat. > + */ > + int pts_wrap_bits; This is maybe a really bad way to export "pts_wrap_bits" but i think User applications could quite reasonably need to know at which point pts wrap. Or whats the max duration for a timebase where pts are still unique or valid. Based on this a user app might warn the user at the begin of transcoding that timestamps will wrap and that with non streaming output, wrap might equal fail. so maybe this should not be declared private without replacement. thx [...]
On 10/9/2020 6:55 PM, Michael Niedermayer wrote: > On Fri, Oct 09, 2020 at 03:04:30PM +0200, Anton Khirnov wrote: >> Specifically: pts_wrap_bits, first_dts, cur_dts. >> They are supposed to be private and are located in the private section >> of AVStream, but ffmpeg.c currently accesses them regardless. They >> should be moved to AVStreamInternal once that bug is fixed. >> >> Remove the marker for the private AVStream section, as there are no >> other accessible fields left there. It has proven highly confusing and >> people kept adding supposedly-public fields into the private section. >> New private per-stream fields should be added to AVStreamInternal. >> --- >> libavformat/avformat.h | 20 +++++++++----------- >> 1 file changed, 9 insertions(+), 11 deletions(-) >> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >> index a01912d654..612791a2eb 100644 >> --- a/libavformat/avformat.h >> +++ b/libavformat/avformat.h >> @@ -1013,22 +1013,16 @@ typedef struct AVStream { >> */ >> AVCodecParameters *codecpar; >> >> - /***************************************************************** >> - * All fields below this line are not part of the public API. They >> - * may not be used outside of libavformat and can be changed and >> - * removed at will. >> - * Internal note: be aware that physically removing these fields >> - * will break ABI. Replace removed fields with dummy fields, and >> - * add new fields to AVStreamInternal. >> - ***************************************************************** >> - */ >> - >> #if LIBAVFORMAT_VERSION_MAJOR < 59 >> // kept for ABI compatibility only, do not access in any way >> void *unused; >> #endif >> > >> - int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */ >> + /** >> + * number of bits in pts (used for wrapping control) >> + * private, do not access from outside libavformat. >> + */ >> + int pts_wrap_bits; > > This is maybe a really bad way to export "pts_wrap_bits" but i think > User applications could quite reasonably need to know at which point pts wrap. > Or whats the max duration for a timebase where pts are still unique or valid. > > Based on this a user app might warn the user at the begin of transcoding that > timestamps will wrap and that with non streaming output, wrap might equal fail. > > so maybe this should not be declared private without replacement. It already is private. This commit doesn't change that. > > thx > > [...] > > > _______________________________________________ > 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". >
On 10/9/2020 6:57 PM, James Almer wrote: > On 10/9/2020 6:55 PM, Michael Niedermayer wrote: >> On Fri, Oct 09, 2020 at 03:04:30PM +0200, Anton Khirnov wrote: >>> Specifically: pts_wrap_bits, first_dts, cur_dts. >>> They are supposed to be private and are located in the private section >>> of AVStream, but ffmpeg.c currently accesses them regardless. They >>> should be moved to AVStreamInternal once that bug is fixed. >>> >>> Remove the marker for the private AVStream section, as there are no >>> other accessible fields left there. It has proven highly confusing and >>> people kept adding supposedly-public fields into the private section. >>> New private per-stream fields should be added to AVStreamInternal. >>> --- >>> libavformat/avformat.h | 20 +++++++++----------- >>> 1 file changed, 9 insertions(+), 11 deletions(-) >>> >>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>> index a01912d654..612791a2eb 100644 >>> --- a/libavformat/avformat.h >>> +++ b/libavformat/avformat.h >>> @@ -1013,22 +1013,16 @@ typedef struct AVStream { >>> */ >>> AVCodecParameters *codecpar; >>> >>> - /***************************************************************** >>> - * All fields below this line are not part of the public API. They >>> - * may not be used outside of libavformat and can be changed and >>> - * removed at will. >>> - * Internal note: be aware that physically removing these fields >>> - * will break ABI. Replace removed fields with dummy fields, and >>> - * add new fields to AVStreamInternal. >>> - ***************************************************************** >>> - */ >>> - >>> #if LIBAVFORMAT_VERSION_MAJOR < 59 >>> // kept for ABI compatibility only, do not access in any way >>> void *unused; >>> #endif >>> >> >>> - int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */ >>> + /** >>> + * number of bits in pts (used for wrapping control) >>> + * private, do not access from outside libavformat. >>> + */ >>> + int pts_wrap_bits; >> >> This is maybe a really bad way to export "pts_wrap_bits" but i think >> User applications could quite reasonably need to know at which point pts wrap. >> Or whats the max duration for a timebase where pts are still unique or valid. >> >> Based on this a user app might warn the user at the begin of transcoding that >> timestamps will wrap and that with non streaming output, wrap might equal fail. >> >> so maybe this should not be declared private without replacement. > > It already is private. This commit doesn't change that. I'm not saying that introducing a proper way to export this same information is a bad idea (It would in fact simply "fixing" the relevant code in ffmpeg.c), just that this commit is merely moving a notification from one part of the struct to another. The field itself remains unaffected after this change. > >> >> thx >> >> [...] >> >> >> _______________________________________________ >> 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". >> >
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index a01912d654..612791a2eb 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -1013,22 +1013,16 @@ typedef struct AVStream { */ AVCodecParameters *codecpar; - /***************************************************************** - * All fields below this line are not part of the public API. They - * may not be used outside of libavformat and can be changed and - * removed at will. - * Internal note: be aware that physically removing these fields - * will break ABI. Replace removed fields with dummy fields, and - * add new fields to AVStreamInternal. - ***************************************************************** - */ - #if LIBAVFORMAT_VERSION_MAJOR < 59 // kept for ABI compatibility only, do not access in any way void *unused; #endif - int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */ + /** + * number of bits in pts (used for wrapping control) + * private, do not access from outside libavformat. + */ + int pts_wrap_bits; // Timestamp generation support: /** @@ -1037,8 +1031,12 @@ typedef struct AVStream { * Initialized when AVCodecParserContext.dts_sync_point >= 0 and * a DTS is received from the underlying container. Otherwise set to * AV_NOPTS_VALUE by default. + * private, do not access from outside libavformat. */ int64_t first_dts; + /** + * private, do not access from outside libavformat. + */ int64_t cur_dts; #if LIBAVFORMAT_VERSION_MAJOR < 59