| Message ID | 20230904150411.56777-1-jamrial@gmail.com |
|---|---|
| Headers | show |
| Series | AVCodecContext and AVCodecParameters side data | expand |
On 9/4/2023 4:03 PM, James Almer wrote:
> 71 files changed, 737 insertions(+), 415 deletions(-)
I see no document updates, commit messages, or deprecation warnings
that would:
* Explain what and why this is happening - this should at the very
least be in the commit message(s), if not a doc somewhere. IRC or
pevious ML threads are not sufficient.
* Warn users they need to update their code to not use stream side data (?).
Will my code just silently change behavior if it was using stream
side data? I legitimately do not know due to the above.
* Any useful doxy for API users or any example aside from function args and
very basic struct info.
I make no comment on the usefulness or utility of the change, but it is these
sorts of things that make downstream API users experiences a little worse.
- Derek
On 9/4/2023 12:37 PM, Derek Buitenhuis wrote: > On 9/4/2023 4:03 PM, James Almer wrote: >> 71 files changed, 737 insertions(+), 415 deletions(-) > > I see no document updates, commit messages, or deprecation warnings > that would: > > * Explain what and why this is happening - this should at the very > least be in the commit message(s), if not a doc somewhere. IRC or > pevious ML threads are not sufficient. I'll mention it in a commit message. The idea is to properly propagate and make available the global side data in AVCodecContext, and no longer have to inject it in the first packet. This way global and frame specific side data is separate, and there's no need to copy and inject entries in several different places. The new struct allows the simple use of a single set of helpers that don't depend on the context containing the side data. > * Warn users they need to update their code to not use stream side data (?). > Will my code just silently change behavior if it was using stream > side data? I legitimately do not know due to the above. How so? This, like any other deprecated field, remains working as it always did until it's removed. The downstream users will see the deprecation warning during compilation, and the doxy for the field mentions the direct replacement. It's standard procedure. I'll add a @deprecated comment to the doxy of av_format_inject_global_side_data() to mention the aforementioned objective. > * Any useful doxy for API users or any example aside from function args and > very basic struct info. The helper functions are basically the same as the packet ones, and the stream ones I'm deprecating. add(), new(), get(), etc. Example usage as usual is in ffmpeg.c, but i think the doxy does a good job explaining what they do. > > I make no comment on the usefulness or utility of the change, but it is these > sorts of things that make downstream API users experiences a little worse. > > - Derek > _______________________________________________ > 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 9/4/2023 5:10 PM, James Almer wrote: >> * Warn users they need to update their code to not use stream side data (?). >> Will my code just silently change behavior if it was using stream >> side data? I legitimately do not know due to the above. > > How so? This, like any other deprecated field, remains working as it > always did until it's removed. The downstream users will see the > deprecation warning during compilation, and the doxy for the field > mentions the direct replacement. It's standard procedure. I mean dowstream users who are relying on the current behavior of checking the first packet or stream side data - will it just cease to behave that way silently? That is, users relying on the *output/result* of the current behavior. > I'll add a @deprecated comment to the doxy of > av_format_inject_global_side_data() to mention the aforementioned objective. > >> * Any useful doxy for API users or any example aside from function args and >> very basic struct info. > > The helper functions are basically the same as the packet ones, and the > stream ones I'm deprecating. add(), new(), get(), etc. Example usage as > usual is in ffmpeg.c, but i think the doxy does a good job explaining > what they do. I think neither ffmpeg.c nor doxy are very good ways to explain to API users both what they should use and why. The doxy relies on the fact you alreay know they exist and you need to use them and for what purpose. ffmpeg.c is the worst example for API users in the known universe. - Derek
On 9/5/2023 10:19 AM, Derek Buitenhuis wrote: > On 9/4/2023 5:10 PM, James Almer wrote: >>> * Warn users they need to update their code to not use stream side data (?). >>> Will my code just silently change behavior if it was using stream >>> side data? I legitimately do not know due to the above. >> >> How so? This, like any other deprecated field, remains working as it >> always did until it's removed. The downstream users will see the >> deprecation warning during compilation, and the doxy for the field >> mentions the direct replacement. It's standard procedure. > > I mean dowstream users who are relying on the current behavior of checking the > first packet or stream side data - will it just cease to behave that > way silently? That is, users relying on the *output/result* of the current > behavior. Users relying on global side data being in the first packet need to call the inject() lavf function to enable said functionality. As that function is now deprecated, they will get the relevant warning and be directed to the global side data API. Nothing is being dropped silently. Their code will behave as usual during the deprecation period. > >> I'll add a @deprecated comment to the doxy of >> av_format_inject_global_side_data() to mention the aforementioned objective. >> >>> * Any useful doxy for API users or any example aside from function args and >>> very basic struct info. >> >> The helper functions are basically the same as the packet ones, and the >> stream ones I'm deprecating. add(), new(), get(), etc. Example usage as >> usual is in ffmpeg.c, but i think the doxy does a good job explaining >> what they do. > > I think neither ffmpeg.c nor doxy are very good ways to explain to API users > both what they should use and why. The doxy relies on the fact you alreay know > they exist and you need to use them and for what purpose. ffmpeg.c is the > worst example for API users in the known universe. What makes this different to every other API that was introduced with relevant documentation and references to it in the deprecacted/replaced API? It's IMO very clear in the doxy: Instead of calling inject() and looking at packet side data, just look at the always available avctx side data. Similarly, instead of looking or filling AVStream.side_data, you look or fill the field in AVStream.codecpar.
On 9/5/2023 2:31 PM, James Almer wrote: > Users relying on global side data being in the first packet need to call > the inject() lavf function to enable said functionality. As that > function is now deprecated, they will get the relevant warning and be > directed to the global side data API. > Nothing is being dropped silently. Their code will behave as usual > during the deprecation period. Thanks for clarifying. > What makes this different to every other API that was introduced with > relevant documentation and references to it in the deprecacted/replaced API? This is true, FFmpeg has terrible doc policies, but it is worth pointing how awful they are once in a while when new ones are added. TBF that is what keeps a whole cottage industry of people employed: Simply knowing the FFmpeg API, as it is non-trivial to know what doc / func / struct you even need for a given problem. > It's IMO very clear in the doxy: Instead of calling inject() and looking > at packet side data, just look at the always available avctx side data. > Similarly, instead of looking or filling AVStream.side_data, you look or > fill the field in AVStream.codecpar. We can agree to disagree. - Derek
Changes since the previous version: - Reverted the undocumented usage of coded_side_data in decoding scenarios. - Changed the signature of some of the helpers to make them extensible. James Almer (17): avcodec/avcodec: add side data to AVCodecContext avcodec/codec_par: add side data to AVCodecParameters avformat/avformat: use the side data from AVStream.codecpar fftools/ffmpeg: stop using AVStream.side_data fftools/ffplay: stop using AVStream.side_data fftools/ffprobe: stop using AVStream.side_data avcodec/hevcdec: check for DOVI configuration record in AVCodecContext side data avcodec/decode: check for global side data in AVCodecContext side data fftools/ffmpeg: stop injecting stream side data in packets fftools/ffplay: stop injecting stream side data in packets Revert "avcodec/mpeg12dec: Do not alter avctx->rc_buffer_size" avformat/demux: propagate the internal decoder's bitrate properties avcodec/mpeg12dec: stop propagating AVCPBProperties side data avcodec/avcodec: deprecate coded_side_data avcodec/utils: move ff_add_cpb_side_data() to encoder code avformat/demux: stop copying the internal AVCodecContext coded_side_data fftools: stop propagating the encoder's coded_side_data fftools/ffmpeg_demux.c | 29 +----- fftools/ffmpeg_enc.c | 31 ++---- fftools/ffmpeg_filter.c | 5 +- fftools/ffmpeg_mux_init.c | 19 ++-- fftools/ffplay.c | 10 +- fftools/ffprobe.c | 30 +++--- libavcodec/avcodec.c | 36 +++++++ libavcodec/avcodec.h | 13 +++ libavcodec/avpacket.c | 99 +++++++++++++++++++ libavcodec/codec_par.c | 43 ++++++++ libavcodec/codec_par.h | 6 ++ libavcodec/decode.c | 55 +++++++++-- libavcodec/decode.h | 2 +- libavcodec/encode.c | 23 +++++ libavcodec/encode.h | 5 + libavcodec/hevcdec.c | 15 ++- libavcodec/internal.h | 5 - libavcodec/libaomenc.c | 2 +- libavcodec/libopenh264enc.c | 2 +- libavcodec/libsvtav1.c | 2 +- libavcodec/libvpxenc.c | 2 +- libavcodec/libx264.c | 2 +- libavcodec/libx265.c | 2 +- libavcodec/mpeg12dec.c | 19 ++-- libavcodec/mpegvideo_enc.c | 2 +- libavcodec/mpegvideo_parser.c | 2 +- libavcodec/nvenc.c | 2 +- libavcodec/packet.h | 68 +++++++++++++ libavcodec/qsvenc.c | 2 +- libavcodec/utils.c | 31 ------ libavcodec/version_major.h | 1 + libavdevice/android_camera.c | 9 +- libavformat/avformat.c | 42 ++------ libavformat/avformat.h | 28 ++++++ libavformat/concatdec.c | 1 - libavformat/dashdec.c | 11 --- libavformat/demux.c | 66 +++++++++---- libavformat/demux_utils.c | 4 + libavformat/dovi_isom.c | 8 +- libavformat/dump.c | 6 +- libavformat/hls.c | 11 --- libavformat/hlsenc.c | 11 ++- libavformat/internal.h | 4 + libavformat/matroskadec.c | 45 ++++----- libavformat/matroskaenc.c | 48 +++++---- libavformat/mov.c | 81 ++++++++------- libavformat/movenc.c | 73 +++++++------- libavformat/mp3enc.c | 8 +- libavformat/mpegenc.c | 18 ++-- libavformat/mpegts.c | 8 +- libavformat/mux.c | 19 ++++ libavformat/mxfdec.c | 22 +++-- libavformat/mxfenc.c | 8 +- libavformat/options.c | 2 + libavformat/replaygain.c | 9 +- libavformat/seek.c | 2 + libavformat/version_major.h | 1 + tests/ref/fate/autorotate | 4 +- tests/ref/fate/copy-trac3074 | 2 +- tests/ref/fate/matroska-avoid-negative-ts | 2 +- tests/ref/fate/matroska-dovi-write-config7 | 2 +- tests/ref/fate/matroska-dovi-write-config8 | 2 +- tests/ref/fate/matroska-encoding-delay | 2 +- .../fate/matroska-mastering-display-metadata | 4 +- tests/ref/fate/matroska-spherical-mono-remux | 4 +- tests/ref/fate/matroska-stereo_mode | 8 +- tests/ref/fate/matroska-vp8-alpha-remux | 2 +- .../ref/fate/mov-mp4-disposition-mpegts-remux | 4 +- tests/ref/fate/mxf-d10-user-comments | 2 +- tests/ref/fate/mxf-remux-applehdr10 | 2 +- tests/ref/fate/vp9-superframe-bsf | 2 +- 71 files changed, 737 insertions(+), 415 deletions(-)