Hi Hans,
Thanks for the comments.
Hans Verkuil wrote:
> On 03/01/2014 02:17 PM, Sakari Ailus wrote:
>> For COPY timestamps, buffer timestamp source flags will traverse the queue
>> untouched.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
>> ---
>> drivers/media/v4l2-core/videobuf2-core.c | 26 +++++++++++++++++++++++++-
>> 1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 3dda083..7afeb6b 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -488,7 +488,22 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>> * Clear any buffer state related flags.
>> */
>> b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
>> - b->flags |= q->timestamp_flags;
>> + if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) ==
>> + V4L2_BUF_FLAG_TIMESTAMP_COPY) {
>> + /*
>> + * For COPY timestamps, we just set the timestamp type
>> + * here. The timestamp source is already in b->flags.
>> + */
>> + b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK;
>> + } else {
>> + /*
>> + * For non-COPY timestamps, drop timestamp source and
>> + * obtain the timestamp type and source from the
>> + * queue.
>> + */
>> + b->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>> + b->flags |= q->timestamp_flags;
>> + }
>
> It's correct, but I would do it a bit differently:
>
> b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK;
> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) !=
> V4L2_BUF_FLAG_TIMESTAMP_COPY) {
> /*
> * For non-COPY timestamps, drop timestamp source and
> * obtain the timestamp type and source from the
> * queue.
> */
> b->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> }
>
> That way it is clearer that the timestamp type is always set and that it is
> just the timestamp source that has special handling.
I was actually pondering between the above and what ended up into the
code. ;-) I'll fix that and change the comment to say:
/*
* For non-COPY timestamps, drop timestamp source bits
* and obtain the timestamp source from the queue.
*/
>
>>
>> switch (vb->state) {
>> case VB2_BUF_STATE_QUEUED:
>> @@ -1031,6 +1046,15 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
>>
>> /* Zero flags that the vb2 core handles */
>> vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS;
>> + if ((vb->vb2_queue->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) !=
>> + V4L2_BUF_FLAG_TIMESTAMP_COPY) {
>> + /*
>> + * Non-COPY timestamps will get their timestamp and
>> + * timestamp source flags from the queue.
>> + */
>> + vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
>> + }
>
> This should be:
>
> if ((vb->vb2_queue->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) !=
> V4L2_BUF_FLAG_TIMESTAMP_COPY || !V4L2_TYPE_IS_OUTPUT(b->type)) {
>
> Capture buffers also need to clear the TSTAMP_SRC flags as they will get it
> from the output queue. In other words: capture buffers never set the timestamp
> source flags, only output buffers can do that if timestamps are copied.
Good point. I'll also change the comment accordingly:
/*
* Non-COPY timestamps and non-OUTPUT queues will get
* their timestamp and timestamp source flags from the
* queue.
*/
> As an aside: the more I think about it, the more I believe that we're not quite
> doing the right thing when it comes to copying timestamps. The problem is that
> TIMESTAMP_COPY is part of the timestamp type mask. It should be a separate bit.
> That way output buffers supply both type and source, and capture buffers give
> those back to the application. Right now you can't copy the timestamp type since
> COPY is part of those types.
Is that a real problem? The timestamp types are related to clocks and in
that sense, "copy" is one of them: it's anything the program queueing it
to the OUTPUT queue has specified. In other words, I don't think the
combination of monotonic (or realtime) and copy would be meaningful.
> We did not really think that through.
At least I don't see a problem with how it's currently implemented. :-)
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
