| Message ID | MpN87y6--7-2@lynne.ee |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] lavu/avframe: add a time_base field | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Thu, 25 Nov 2021, Lynne wrote: > This adds a time_base field (currently unused), analogue to the > AVPacket.time_base field. > This allows for API clients to exchange AVFrames directly, without > needing to plumb extra data from sources via side mechanisms. The objections raised before still stand I believe, and again, the main concern is that the API user won't know when to use the packet or frame timebase and when to use the frame/packet source timebase. You write this in the doxy: > Time base for the timestamps in this frame. May be 0, in which case the > time_base from the frame source should be used. One could easily think - based on this text alone - that every user of avcodec_receive_frame should check if AVFrame->time_base is 0 to calculate real PTS... I'd be a lot more willing to accept this if you could document explicitly that AVFrame->time_base (and AVPacket->time_base) is not used by the API right now, and is similar to e.g. *opaque field. And later, if by using some magic flag or new API or whatever it turns out to make sense to actually use AVPacket/AVFrame time base, the doxy text can be extended accordingly. Regards, Marton
25 Nov 2021, 23:49 by cus@passwd.hu: > > > On Thu, 25 Nov 2021, Lynne wrote: > >> This adds a time_base field (currently unused), analogue to the >> AVPacket.time_base field. >> This allows for API clients to exchange AVFrames directly, without >> needing to plumb extra data from sources via side mechanisms. >> > > The objections raised before still stand I believe, and again, the main concern is that the API user won't know when to use the packet or frame timebase and when to use the frame/packet source timebase. > > You write this in the doxy: > >> Time base for the timestamps in this frame. May be 0, in which case the >> time_base from the frame source should be used. >> > > One could easily think - based on this text alone - that every user of avcodec_receive_frame should check if AVFrame->time_base is 0 to calculate real PTS... > > I'd be a lot more willing to accept this if you could document explicitly that AVFrame->time_base (and AVPacket->time_base) is not used by the API right now, and is similar to e.g. *opaque field. And later, if by using some magic flag or new API or whatever it turns out to make sense to actually use AVPacket/AVFrame time base, the doxy text can be extended accordingly. > So you'd like for the field to either be fully opaque or fully working (by always having the correct time_base value)? That sounds fair, description changed: "Time base for the timestamps in this frame. Currently unused by any API, users can set this and use it as an opaque field. In the future, this field may be set by the API for you, but its value will always be ignored."
On Fri, 26 Nov 2021, Lynne wrote: > 25 Nov 2021, 23:49 by cus@passwd.hu: > >> >> >> On Thu, 25 Nov 2021, Lynne wrote: >> >>> This adds a time_base field (currently unused), analogue to the >>> AVPacket.time_base field. >>> This allows for API clients to exchange AVFrames directly, without >>> needing to plumb extra data from sources via side mechanisms. >>> >> >> The objections raised before still stand I believe, and again, the main concern is that the API user won't know when to use the packet or frame timebase and when to use the frame/packet source timebase. >> >> You write this in the doxy: >> >>> Time base for the timestamps in this frame. May be 0, in which case the >>> time_base from the frame source should be used. >>> >> >> One could easily think - based on this text alone - that every user of avcodec_receive_frame should check if AVFrame->time_base is 0 to calculate real PTS... >> >> I'd be a lot more willing to accept this if you could document explicitly that AVFrame->time_base (and AVPacket->time_base) is not used by the API right now, and is similar to e.g. *opaque field. And later, if by using some magic flag or new API or whatever it turns out to make sense to actually use AVPacket/AVFrame time base, the doxy text can be extended accordingly. >> > > So you'd like for the field to either be fully opaque or fully working (by always > having the correct time_base value)? > That sounds fair, description changed: > "Time base for the timestamps in this frame. Currently unused by any API, > users can set this and use it as an opaque field. In the future, this field may > be set by the API for you, but its value will always be ignored." Fine by me with that text. Please update AVPacket time_base with something similar as well. Thanks, Marton
Quoting Lynne (2021-11-26 09:00:59) > 25 Nov 2021, 23:49 by cus@passwd.hu: > > > > > > > On Thu, 25 Nov 2021, Lynne wrote: > > > >> This adds a time_base field (currently unused), analogue to the > >> AVPacket.time_base field. > >> This allows for API clients to exchange AVFrames directly, without > >> needing to plumb extra data from sources via side mechanisms. > >> > > > > The objections raised before still stand I believe, and again, the main concern is that the API user won't know when to use the packet or frame timebase and when to use the frame/packet source timebase. > > > > You write this in the doxy: > > > >> Time base for the timestamps in this frame. May be 0, in which case the > >> time_base from the frame source should be used. > >> > > > > One could easily think - based on this text alone - that every user of avcodec_receive_frame should check if AVFrame->time_base is 0 to calculate real PTS... > > > > I'd be a lot more willing to accept this if you could document explicitly that AVFrame->time_base (and AVPacket->time_base) is not used by the API right now, and is similar to e.g. *opaque field. And later, if by using some magic flag or new API or whatever it turns out to make sense to actually use AVPacket/AVFrame time base, the doxy text can be extended accordingly. > > > > So you'd like for the field to either be fully opaque or fully working (by always > having the correct time_base value)? > That sounds fair, description changed: > "Time base for the timestamps in this frame. Currently unused by any API, > users can set this and use it as an opaque field. In the future, this field may > be set by the API for you, but its value will always be ignored." Always is a bit too strong, presumably we do want to start using that field eventually. How about something like: In the future, this field may be set on frames output by decoders or filters, but its value will be by default ignored on input to encoders or filters.
1 Dec 2021, 16:47 by anton@khirnov.net: > Quoting Lynne (2021-11-26 09:00:59) > >> 25 Nov 2021, 23:49 by cus@passwd.hu: >> >> > >> > >> > On Thu, 25 Nov 2021, Lynne wrote: >> > >> >> This adds a time_base field (currently unused), analogue to the >> >> AVPacket.time_base field. >> >> This allows for API clients to exchange AVFrames directly, without >> >> needing to plumb extra data from sources via side mechanisms. >> >> >> > >> > The objections raised before still stand I believe, and again, the main concern is that the API user won't know when to use the packet or frame timebase and when to use the frame/packet source timebase. >> > >> > You write this in the doxy: >> > >> >> Time base for the timestamps in this frame. May be 0, in which case the >> >> time_base from the frame source should be used. >> >> >> > >> > One could easily think - based on this text alone - that every user of avcodec_receive_frame should check if AVFrame->time_base is 0 to calculate real PTS... >> > >> > I'd be a lot more willing to accept this if you could document explicitly that AVFrame->time_base (and AVPacket->time_base) is not used by the API right now, and is similar to e.g. *opaque field. And later, if by using some magic flag or new API or whatever it turns out to make sense to actually use AVPacket/AVFrame time base, the doxy text can be extended accordingly. >> > >> >> So you'd like for the field to either be fully opaque or fully working (by always >> having the correct time_base value)? >> That sounds fair, description changed: >> "Time base for the timestamps in this frame. Currently unused by any API, >> users can set this and use it as an opaque field. In the future, this field may >> be set by the API for you, but its value will always be ignored." >> > > Always is a bit too strong, presumably we do want to start using that > field eventually. > > How about something like: > In the future, this field may be set on frames output by decoders or > filters, but its value will be by default ignored on input to encoders > or filters. > Yes, that's much better and allows us to extend it like avpacket's field. I've changed it locally. I think this has been on the ML long enough, so I'll push it in 2 days with the amended description so it makes it in 5.0. I'll also send a patch to change the description to the AVPacket's field to this soon.
1 Dec 2021, 21:11 by dev@lynne.ee: > 1 Dec 2021, 16:47 by anton@khirnov.net: > >> Quoting Lynne (2021-11-26 09:00:59) >> >>> 25 Nov 2021, 23:49 by cus@passwd.hu: >>> >>> > >>> > >>> > On Thu, 25 Nov 2021, Lynne wrote: >>> > >>> >> This adds a time_base field (currently unused), analogue to the >>> >> AVPacket.time_base field. >>> >> This allows for API clients to exchange AVFrames directly, without >>> >> needing to plumb extra data from sources via side mechanisms. >>> >> >>> > >>> > The objections raised before still stand I believe, and again, the main concern is that the API user won't know when to use the packet or frame timebase and when to use the frame/packet source timebase. >>> > >>> > You write this in the doxy: >>> > >>> >> Time base for the timestamps in this frame. May be 0, in which case the >>> >> time_base from the frame source should be used. >>> >> >>> > >>> > One could easily think - based on this text alone - that every user of avcodec_receive_frame should check if AVFrame->time_base is 0 to calculate real PTS... >>> > >>> > I'd be a lot more willing to accept this if you could document explicitly that AVFrame->time_base (and AVPacket->time_base) is not used by the API right now, and is similar to e.g. *opaque field. And later, if by using some magic flag or new API or whatever it turns out to make sense to actually use AVPacket/AVFrame time base, the doxy text can be extended accordingly. >>> > >>> >>> So you'd like for the field to either be fully opaque or fully working (by always >>> having the correct time_base value)? >>> That sounds fair, description changed: >>> "Time base for the timestamps in this frame. Currently unused by any API, >>> users can set this and use it as an opaque field. In the future, this field may >>> be set by the API for you, but its value will always be ignored." >>> >> >> Always is a bit too strong, presumably we do want to start using that >> field eventually. >> >> How about something like: >> In the future, this field may be set on frames output by decoders or >> filters, but its value will be by default ignored on input to encoders >> or filters. >> > > Yes, that's much better and allows us to extend it like avpacket's field. > I've changed it locally. I think this has been on the ML long enough, > so I'll push it in 2 days with the amended description so it makes it in 5.0. > I'll also send a patch to change the description to the AVPacket's field > to this soon. > Thanks everyone for the review, patch pushed along with APIchanges and minor bump. I've sent a patch to add the same description for the AVPacket.time_base field, and I'll write something up during the weekend to set them where possible and easy.
diff --git a/libavutil/frame.c b/libavutil/frame.c index d4d3ad6988..64eed5b0dd 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -63,6 +63,7 @@ static void get_frame_defaults(AVFrame *frame) frame->pkt_duration = 0; frame->pkt_pos = -1; frame->pkt_size = -1; + frame->time_base = (AVRational){ 0, 1 }; frame->key_frame = 1; frame->sample_aspect_ratio = (AVRational){ 0, 1 }; frame->format = -1; /* unknown */ @@ -278,6 +279,7 @@ static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy) dst->pkt_pos = src->pkt_pos; dst->pkt_size = src->pkt_size; dst->pkt_duration = src->pkt_duration; + dst->time_base = src->time_base; dst->reordered_opaque = src->reordered_opaque; dst->quality = src->quality; dst->best_effort_timestamp = src->best_effort_timestamp; diff --git a/libavutil/frame.h b/libavutil/frame.h index 753234792e..66fe3070f8 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -421,6 +421,12 @@ typedef struct AVFrame { */ int64_t pkt_dts; + /** + * Time base for the timestamps in this frame. May be 0, in which case the + * time_base from the frame source should be used. + */ + AVRational time_base; + /** * picture number in bitstream order */