| Message ID | 20220713091725.16638-6-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/9] lavu/frame: add a duration field to AVFrame | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure_x86 | warning | Failed to apply patch |
You should probably add new entries to the schema file instead. https://git.videolan.org/?p=ffmpeg.git;a=blob;f=doc/ffprobe.xsd;h=3af621a17ae884adfeacb7cd50c60e1553808188;hb=HEAD#l93 Once frame->pkt_duration is gone, ffprobe shouldn't keep printing a frame->duration value as "pkt_duration" and "pkt_duration_time". Also, if frame->duration is supposed to be able to have values other than those we wrote to pkt_duration, maybe keep printing the latter with the existing schema entries until it's all gone (You can use AV_NOWARN_DEPRECATED() to shut compilers up). On 7/13/2022 6:17 AM, Anton Khirnov wrote: > --- > fftools/ffprobe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > index f156663019..a041241e1e 100644 > --- a/fftools/ffprobe.c > +++ b/fftools/ffprobe.c > @@ -2570,8 +2570,8 @@ static void show_frame(WriterContext *w, AVFrame *frame, AVStream *stream, > print_time("pkt_dts_time", frame->pkt_dts, &stream->time_base); > print_ts ("best_effort_timestamp", frame->best_effort_timestamp); > print_time("best_effort_timestamp_time", frame->best_effort_timestamp, &stream->time_base); > - print_duration_ts ("pkt_duration", frame->pkt_duration); > - print_duration_time("pkt_duration_time", frame->pkt_duration, &stream->time_base); > + print_duration_ts ("pkt_duration", frame->duration); > + print_duration_time("pkt_duration_time", frame->duration, &stream->time_base); > if (frame->pkt_pos != -1) print_fmt ("pkt_pos", "%"PRId64, frame->pkt_pos); > else print_str_opt("pkt_pos", "N/A"); > if (frame->pkt_size != -1) print_val ("pkt_size", frame->pkt_size, unit_byte_str);
Quoting James Almer (2022-07-13 14:39:20) > You should probably add new entries to the schema file instead. > > https://git.videolan.org/?p=ffmpeg.git;a=blob;f=doc/ffprobe.xsd;h=3af621a17ae884adfeacb7cd50c60e1553808188;hb=HEAD#l93 > > Once frame->pkt_duration is gone, ffprobe shouldn't keep printing a > frame->duration value as "pkt_duration" and "pkt_duration_time". > Also, if frame->duration is supposed to be able to have values other > than those we wrote to pkt_duration, maybe keep printing the latter with > the existing schema entries until it's all gone (You can use > AV_NOWARN_DEPRECATED() to shut compilers up). Well, the whole idea that values printed by ffprobe should precisely mirror the libav* API, up to the field names of our structs, seems quite questionable to me, IMO it makes more sense for it to provide a layer of abstraction over the libraries. But then again I neither maintain ffprobe, nor use it very much, so I wanted to keep changes to a minimum. If nobody has other plans for dealing with this then I can certainly make the changes you're sugesting.
On 7/14/2022 6:34 AM, Anton Khirnov wrote: > Quoting James Almer (2022-07-13 14:39:20) >> You should probably add new entries to the schema file instead. >> >> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=doc/ffprobe.xsd;h=3af621a17ae884adfeacb7cd50c60e1553808188;hb=HEAD#l93 >> >> Once frame->pkt_duration is gone, ffprobe shouldn't keep printing a >> frame->duration value as "pkt_duration" and "pkt_duration_time". >> Also, if frame->duration is supposed to be able to have values other >> than those we wrote to pkt_duration, maybe keep printing the latter with >> the existing schema entries until it's all gone (You can use >> AV_NOWARN_DEPRECATED() to shut compilers up). > > Well, the whole idea that values printed by ffprobe should precisely > mirror the libav* API, up to the field names of our structs, seems quite > questionable to me, IMO it makes more sense for it to provide a layer of > abstraction over the libraries. But then again I neither maintain > ffprobe, nor use it very much, so I wanted to keep changes to a minimum. > If nobody has other plans for dealing with this then I can certainly > make the changes you're sugesting. ffprobe used to print pkt_pts until it was removed, so printing a field that does not exist (once it's removed) where the new values assigned to it have potentially slightly different semantics than the old ones does not seem like a good idea.
diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index f156663019..a041241e1e 100644 --- a/fftools/ffprobe.c +++ b/fftools/ffprobe.c @@ -2570,8 +2570,8 @@ static void show_frame(WriterContext *w, AVFrame *frame, AVStream *stream, print_time("pkt_dts_time", frame->pkt_dts, &stream->time_base); print_ts ("best_effort_timestamp", frame->best_effort_timestamp); print_time("best_effort_timestamp_time", frame->best_effort_timestamp, &stream->time_base); - print_duration_ts ("pkt_duration", frame->pkt_duration); - print_duration_time("pkt_duration_time", frame->pkt_duration, &stream->time_base); + print_duration_ts ("pkt_duration", frame->duration); + print_duration_time("pkt_duration_time", frame->duration, &stream->time_base); if (frame->pkt_pos != -1) print_fmt ("pkt_pos", "%"PRId64, frame->pkt_pos); else print_str_opt("pkt_pos", "N/A"); if (frame->pkt_size != -1) print_val ("pkt_size", frame->pkt_size, unit_byte_str);