| Message ID | 1393679828-25878-7-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sakari, I believe this needs some more work. See comments below: 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@iki.fi> > --- > 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. > > 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. 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. We did not really think that through. Regards, Hans > + > if (V4L2_TYPE_IS_OUTPUT(b->type)) { > /* > * For output buffers mask out the timecode flag: > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@iki.fi> >> --- >> 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. :-)
On 03/01/2014 05:51 PM, Sakari Ailus wrote: > 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@iki.fi> >>> --- >>> 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. :-) Well, the problem is that when you capture from hardware you get a timestamp and a timestamp type (i.e. which clock did the timestamp use?). If you then pass that captured stream through a m2m device for postprocessing you will have lost the timestamp type on the other side of the m2m device since it will be replaced with COPY. That's not right IMHO. What should happen I think for m2m copy devices is the following: Initially after creating the buffers the timestamp type is set to COPY for both capture and output buffers. You fill in the output buffer with timestamp, timestamp source and timestamp type and call QBUF. At the moment QUERYBUF will return what you put in there. Dequeuing that buffer on the capture side will give back what you put in. On the capture side the timestamp type should be set to COPY when you queue the empty buffer and it will stay that way until the buffer is filled up with the new frame in the m2m device. I'm not sure what to do on the output side when dequeuing a buffer: keep the values you put in when queuing it, or replace it with COPY. I think keeping it is better, you might want to queue the same buffer to another m2m device so you don't want to set up those values again. Yeah, I think that would make sense. COPY can still be part of the timestamp type mask, but it's just replaced by the user's timestamp settings as you would expect. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + } 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; + } + if (V4L2_TYPE_IS_OUTPUT(b->type)) { /* * For output buffers mask out the timecode flag:
For COPY timestamps, buffer timestamp source flags will traverse the queue untouched. Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> --- drivers/media/v4l2-core/videobuf2-core.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)