[FFmpeg-devel] [PATCH v2 1/4] avformat: add av_program_copy()
Gyan Doshi
ffmpeg at gyani.pro
Thu Feb 16 09:58:10 EET 2023
On 2023-02-16 12:22 pm, Andreas Rheinhardt wrote:
> Gyan Doshi:
>> Helper to transfer programs from one muxing context to another.
>> ---
>> doc/APIchanges | 3 ++
>> libavformat/avformat.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>> libavformat/avformat.h | 14 +++++++++
>> libavformat/version.h | 2 +-
>> 4 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 6baf914760..4916d1abe8 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -14,6 +14,9 @@ libavutil: 2021-04-27
>>
>> API changes, most recent first:
>>
>> +2023-02-xx - xxxxxxxxxx - lavf 59.39.100 - avformat.h
>> + Add av_program_copy()
>> +
>> 2023-0x-xx - xxxxxxxxxx - lavc 59.63.100
>> Allow AV_CODEC_FLAG_COPY_OPAQUE to be used with decoders.
>>
>> diff --git a/libavformat/avformat.c b/libavformat/avformat.c
>> index 19c7219471..d3c8def170 100644
>> --- a/libavformat/avformat.c
>> +++ b/libavformat/avformat.c
>> @@ -359,6 +359,76 @@ void av_program_add_stream_index(AVFormatContext *ac, int progid, unsigned idx)
>> }
>> }
>>
>> +int av_program_copy(AVFormatContext *dst, AVFormatContext *src, int progid, int overwrite)
>> +{
>> + AVProgram *src_prog = NULL;
> Should be const.
>
>> + AVProgram *dst_prog = NULL;
>> + int i, j, ret;
>> + int idx = -1;
>> +
>> + for (i = 0; i < src->nb_programs; i++)
> 1. nb_programs is unsigned and therefore the iterator should be, too.
> 2. Use a smaller scope for the iterator.
> (Same for all other iterators.)
>
>> + if (src->programs[i]->id == progid)
>> + src_prog = src->programs[i];
>> +
>> + if (!src_prog) {
>> + av_log(src, AV_LOG_ERROR, "source program not found: id=0x%04x\n", progid);
>> + return AVERROR(EINVAL);
>> + }
>> +
>> + for (i = 0; i < dst->nb_programs; i++) {
>> + if (dst->programs[i]->id == progid)
>> + idx = i;
>> + }
>> +
>> + if (idx >= 0 && !overwrite) {
>> + av_log(src, AV_LOG_ERROR, "target muxer already has program with id=0x%04x; not overwriting\n", progid);
>> + return AVERROR(EINVAL);
> This should not be an error.
>
>> + }
>> +
>> + av_log(src, AV_LOG_TRACE, "%s program: id=0x%04x\n", idx >= 0 ? "overwriting" : "copying", progid);
>> +
>> + if (idx >= 0) {
>> + dst_prog = dst->programs[idx];
>> + av_dict_free(&dst_prog->metadata);
>> + av_freep(&dst_prog->stream_index);
>> + dst_prog->nb_stream_indexes = 0;
>> + } else {
>> + dst_prog = av_mallocz(sizeof(*dst_prog));
>> + if (!dst_prog)
>> + return AVERROR(ENOMEM);
>> + ret = av_dynarray_add_nofree(&dst->programs, &dst->nb_programs, dst_prog);
> av_dynarray_add_nofree() presumes that dst->programs points to a buffer
> for a power-of-two pointers (or to NULL); what if a user has reallocated
> dst->programs himself and moved AVPrograms from another AVFormatContext
> manually? Do we treat this as API violation?
That scenario should be treated the same way as in av_new_program().
But I presume that if a user knows of and uses this helper, they won't
have done that.
I can add it in the doxy as a caveat.
>
>> + if (ret < 0) {
>> + av_free(dst_prog);
>> + return AVERROR(ENOMEM);
>> + }
>> + }
>> +
>> + /* public fields */
>> + dst_prog->id = src_prog->id;
>> + dst_prog->flags = src_prog->flags;
>> + dst_prog->discard = src_prog->discard;
>> + dst_prog->program_num = src_prog->program_num;
>> + dst_prog->pmt_pid = src_prog->pmt_pid;
>> + dst_prog->pcr_pid = src_prog->pcr_pid;
>> + dst_prog->pmt_version = src_prog->pmt_version;
>> +
>> + for (i = 0; i < dst->nb_streams; i++) {
>> + for (j = 0; j < src_prog->nb_stream_indexes; j++)
>> + if (dst->streams[i]->id == src->streams[src_prog->stream_index[j]]->id)
> The documentation should mention that AVStream.id is used for
> stream-matching.
>
>> + av_program_add_stream_index(dst, dst_prog->id, i);
> This involves a realloction whose success can't be checked due to a
> design bug in av_program_add_stream_index(). Add an
> ff_program_add_stream_index() without this design bug and turn
> av_program_add_stream_index() into a wrapper for it.
Isn't it better to deprecate this and add av_program_add_stream_index2()
without the bug?
>
>> + }
>> +
>> + av_dict_copy(&dst_prog->metadata, src_prog->metadata, 0);
> Missing error check.
Ok, but as a note, most usages (80%+) in the codebase omit the check,
including
lavu/opt.c, lavu/tests/dict.c, lavf/mux.c and fftools/*
Do I restore the destination program (when overwriting) or bow out?
>
>> +
>> + /* private fields */
>> + dst_prog->start_time = src_prog->start_time;
>> + dst_prog->end_time = src_prog->end_time;
>> + dst_prog->pts_wrap_reference = src_prog->pts_wrap_reference;
>> + dst_prog->pts_wrap_behavior = src_prog->pts_wrap_behavior;
> These private fields are demuxer-only; there is no need to copy them for
> muxers.
>
>> +
>> + return 0;
>> +}
>> +
>> AVProgram *av_find_program_from_stream(AVFormatContext *ic, AVProgram *last, int s)
>> {
>> for (unsigned i = 0; i < ic->nb_programs; i++) {
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 1d97d56ac5..2b7ed3abd8 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -1996,6 +1996,20 @@ uint8_t *av_stream_get_side_data(const AVStream *stream,
>>
>> AVProgram *av_new_program(AVFormatContext *s, int id);
>>
>> +/**
>> + * Copy an AVProgram from one AVFormatContext to another.
>> + *
>> + * @param dst pointer to the target muxer context
>> + * @param src pointer to the source muxer context
>> + * @param progid ID of the program to be copied
>> + * @param overwrite whether to overwrite if target muxer already
>> + * contains a program with the same ID
>> + *
>> + * @return 0 in case of success, a negative AVERROR code in case of
>> + * failure
> Better make >= 0 for success for extensibility.
>
>> + */
>> +int av_program_copy(AVFormatContext *dst, AVFormatContext *src, int progid, int overwrite);
>> +
> 3. Why is this public? Which user is supposed to need this?
Someone implementing tee or similar manually.
Ok to the rest.
Regards,
Gyan
> 4. src should be const.
>
>> /**
>> * @}
>> */
>> diff --git a/libavformat/version.h b/libavformat/version.h
>> index 134cdb2b89..9aba356e09 100644
>> --- a/libavformat/version.h
>> +++ b/libavformat/version.h
>> @@ -31,7 +31,7 @@
>>
>> #include "version_major.h"
>>
>> -#define LIBAVFORMAT_VERSION_MINOR 38
>> +#define LIBAVFORMAT_VERSION_MINOR 39
>> #define LIBAVFORMAT_VERSION_MICRO 100
>>
>> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list