diff mbox series

[FFmpeg-devel,v2] lavu/avframe: add a time_base field

Message ID MpN87y6--7-2@lynne.ee
State New
Headers show
Series [FFmpeg-devel,v2] lavu/avframe: add a time_base field
Related show

Checks

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

Commit Message

Lynne Nov. 25, 2021, 5:28 p.m. UTC
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.

Patch attached.
Subject: [PATCH] lavu/avframe: add a time_base field

This adds a time_base field (currently unused), analogue to the
AVPacket.time_base field.
---
 libavutil/frame.c | 2 ++
 libavutil/frame.h | 6 ++++++
 2 files changed, 8 insertions(+)

Comments

Marton Balint Nov. 25, 2021, 10:49 p.m. UTC | #1
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
Lynne Nov. 26, 2021, 8 a.m. UTC | #2
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."
Marton Balint Nov. 29, 2021, 8:23 p.m. UTC | #3
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
Anton Khirnov Dec. 1, 2021, 3:47 p.m. UTC | #4
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.
Lynne Dec. 1, 2021, 8:11 p.m. UTC | #5
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.
Lynne Dec. 3, 2021, 9:49 p.m. UTC | #6
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 mbox series

Patch

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
      */