| Message ID | 1393690690-5004-8-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sakari, Thank you for the patch. Given that the timestamp type and source are not supposed to change during streaming, do we really need to print them for every frame ? On Saturday 01 March 2014 18:18:08 Sakari Ailus wrote: > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > --- > yavta.c | 52 ++++++++++++++++++++++++++++++---------------------- > 1 file changed, 30 insertions(+), 22 deletions(-) > > diff --git a/yavta.c b/yavta.c > index 71c1477..224405d 100644 > --- a/yavta.c > +++ b/yavta.c > @@ -445,6 +445,30 @@ static int video_set_framerate(struct device *dev, > struct v4l2_fract *time_per_f return 0; > } > > +static void get_ts_flags(uint32_t flags, const char **ts_type, const char > **ts_source) +{ > + switch (flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) { > + case V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN: > + *ts_type = "unknown"; > + break; > + case V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC: > + *ts_type = "monotonic"; > + break; > + default: > + *ts_type = "invalid"; > + } > + switch (flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK) { > + case V4L2_BUF_FLAG_TSTAMP_SRC_EOF: > + *ts_source = "EoF"; > + break; > + case V4L2_BUF_FLAG_TSTAMP_SRC_SOE: > + *ts_source = "SoE"; > + break; > + default: > + *ts_source = "invalid"; > + } > +} > + > static int video_alloc_buffers(struct device *dev, int nbufs, > unsigned int offset, unsigned int padding) > { > @@ -488,26 +512,7 @@ static int video_alloc_buffers(struct device *dev, int > nbufs, strerror(errno), errno); > return ret; > } > - switch (buf.flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) { > - case V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN: > - ts_type = "unknown"; > - break; > - case V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC: > - ts_type = "monotonic"; > - break; > - default: > - ts_type = "invalid"; > - } > - switch (buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK) { > - case V4L2_BUF_FLAG_TSTAMP_SRC_EOF: > - ts_source = "EoF"; > - break; > - case V4L2_BUF_FLAG_TSTAMP_SRC_SOE: > - ts_source = "SoE"; > - break; > - default: > - ts_source = "invalid"; > - } > + get_ts_flags(buf.flags, &ts_type, &ts_source); > printf("length: %u offset: %u timestamp type/source: %s/%s\n", > buf.length, buf.m.offset, ts_type, ts_source); > > @@ -1131,6 +1136,7 @@ static int video_do_capture(struct device *dev, > unsigned int nframes, last.tv_usec = start.tv_nsec / 1000; > > for (i = 0; i < nframes; ++i) { > + const char *ts_type, *ts_source; > /* Dequeue a buffer. */ > memset(&buf, 0, sizeof buf); > buf.type = dev->type; > @@ -1163,10 +1169,12 @@ static int video_do_capture(struct device *dev, > unsigned int nframes, fps = fps ? 1000000.0 / fps : 0.0; > > clock_gettime(CLOCK_MONOTONIC, &ts); > - printf("%u (%u) [%c] %u %u bytes %ld.%06ld %ld.%06ld %.3f fps\n", i, > buf.index, + get_ts_flags(buf.flags, &ts_type, &ts_source); > + printf("%u (%u) [%c] %u %u bytes %ld.%06ld %ld.%06ld %.3f fps tstamp > type/src %s/%s\n", i, buf.index, (buf.flags & V4L2_BUF_FLAG_ERROR) ? 'E' : > '-', > buf.sequence, buf.bytesused, buf.timestamp.tv_sec, > - buf.timestamp.tv_usec, ts.tv_sec, ts.tv_nsec/1000, fps); > + buf.timestamp.tv_usec, ts.tv_sec, ts.tv_nsec/1000, fps, > + ts_type, ts_source); > > last = buf.timestamp;
Hi Laurent, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > Given that the timestamp type and source are not supposed to change during > streaming, do we really need to print them for every frame ? When processing frames from memory to memory (COPY timestamp type), the it is entirely possible that the timestamp source changes as the flags are copied from the OUTPUT buffer to the CAPTURE buffer. These patches do not support it but it is allowed. One option would be to print the source on every frame only when the type is COPY. For a program like yavta this might be overly sophisticated IMO. :-)
On Thursday 10 April 2014 21:58:41 Sakari Ailus wrote: > Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > Given that the timestamp type and source are not supposed to change during > > streaming, do we really need to print them for every frame ? > > When processing frames from memory to memory (COPY timestamp type), the > it is entirely possible that the timestamp source changes as the flags > are copied from the OUTPUT buffer to the CAPTURE buffer. It's possible, but is it allowed by the V4L2 API ? > These patches do not support it but it is allowed. > > One option would be to print the source on every frame only when the > type is COPY. For a program like yavta this might be overly > sophisticated IMO. :-) My concern is that this makes the lines output by yavta pretty long.
Laurent Pinchart wrote: > On Thursday 10 April 2014 21:58:41 Sakari Ailus wrote: >> Laurent Pinchart wrote: >>> Hi Sakari, >>> >>> Thank you for the patch. >>> >>> Given that the timestamp type and source are not supposed to change during >>> streaming, do we really need to print them for every frame ? >> >> When processing frames from memory to memory (COPY timestamp type), the >> it is entirely possible that the timestamp source changes as the flags >> are copied from the OUTPUT buffer to the CAPTURE buffer. > > It's possible, but is it allowed by the V4L2 API ? The spec states that: "The V4L2_BUF_FLAG_TIMESTAMP_COPY timestamp type which is used by e.g. on mem-to-mem devices is an exception to the rule: the timestamp source flags are copied from the OUTPUT video buffer to the CAPTURE video buffer." >> These patches do not support it but it is allowed. >> >> One option would be to print the source on every frame only when the >> type is COPY. For a program like yavta this might be overly >> sophisticated IMO. :-) > > My concern is that this makes the lines output by yavta pretty long. True as well. I could remove "type/src " from the timestamp source information. That's mostly redundant anyway. Then we shouldn't exceed 80 characters per line that easily anymore. Could this be the time to add a "verbose" option? :-)
Hi Sakari, On Friday 11 April 2014 01:36:03 Sakari Ailus wrote: > Laurent Pinchart wrote: > > On Thursday 10 April 2014 21:58:41 Sakari Ailus wrote: > >> Laurent Pinchart wrote: > >>> Hi Sakari, > >>> > >>> Thank you for the patch. > >>> > >>> Given that the timestamp type and source are not supposed to change > >>> during streaming, do we really need to print them for every frame ? > >> > >> When processing frames from memory to memory (COPY timestamp type), the > >> it is entirely possible that the timestamp source changes as the flags > >> are copied from the OUTPUT buffer to the CAPTURE buffer. > > > > It's possible, but is it allowed by the V4L2 API ? > > The spec states that: > > "The V4L2_BUF_FLAG_TIMESTAMP_COPY timestamp type which is used by e.g. on > mem-to-mem devices is an exception to the rule: the timestamp source flags > are copied from the OUTPUT video buffer to the CAPTURE video buffer." > > >> These patches do not support it but it is allowed. > >> > >> One option would be to print the source on every frame only when the > >> type is COPY. For a program like yavta this might be overly > >> sophisticated IMO. :-) > > > > My concern is that this makes the lines output by yavta pretty long. > > True as well. I could remove "type/src " from the timestamp source > information. That's mostly redundant anyway. Then we shouldn't exceed 80 > characters per line that easily anymore. I think that would be better. > Could this be the time to add a "verbose" option? :-) Possibly, but then we'll need to discuss what information should be printed in verbose mode only :-)
diff --git a/yavta.c b/yavta.c index 71c1477..224405d 100644 --- a/yavta.c +++ b/yavta.c @@ -445,6 +445,30 @@ static int video_set_framerate(struct device *dev, struct v4l2_fract *time_per_f return 0; } +static void get_ts_flags(uint32_t flags, const char **ts_type, const char **ts_source) +{ + switch (flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) { + case V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN: + *ts_type = "unknown"; + break; + case V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC: + *ts_type = "monotonic"; + break; + default: + *ts_type = "invalid"; + } + switch (flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK) { + case V4L2_BUF_FLAG_TSTAMP_SRC_EOF: + *ts_source = "EoF"; + break; + case V4L2_BUF_FLAG_TSTAMP_SRC_SOE: + *ts_source = "SoE"; + break; + default: + *ts_source = "invalid"; + } +} + static int video_alloc_buffers(struct device *dev, int nbufs, unsigned int offset, unsigned int padding) { @@ -488,26 +512,7 @@ static int video_alloc_buffers(struct device *dev, int nbufs, strerror(errno), errno); return ret; } - switch (buf.flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) { - case V4L2_BUF_FLAG_TIMESTAMP_UNKNOWN: - ts_type = "unknown"; - break; - case V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC: - ts_type = "monotonic"; - break; - default: - ts_type = "invalid"; - } - switch (buf.flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK) { - case V4L2_BUF_FLAG_TSTAMP_SRC_EOF: - ts_source = "EoF"; - break; - case V4L2_BUF_FLAG_TSTAMP_SRC_SOE: - ts_source = "SoE"; - break; - default: - ts_source = "invalid"; - } + get_ts_flags(buf.flags, &ts_type, &ts_source); printf("length: %u offset: %u timestamp type/source: %s/%s\n", buf.length, buf.m.offset, ts_type, ts_source); @@ -1131,6 +1136,7 @@ static int video_do_capture(struct device *dev, unsigned int nframes, last.tv_usec = start.tv_nsec / 1000; for (i = 0; i < nframes; ++i) { + const char *ts_type, *ts_source; /* Dequeue a buffer. */ memset(&buf, 0, sizeof buf); buf.type = dev->type; @@ -1163,10 +1169,12 @@ static int video_do_capture(struct device *dev, unsigned int nframes, fps = fps ? 1000000.0 / fps : 0.0; clock_gettime(CLOCK_MONOTONIC, &ts); - printf("%u (%u) [%c] %u %u bytes %ld.%06ld %ld.%06ld %.3f fps\n", i, buf.index, + get_ts_flags(buf.flags, &ts_type, &ts_source); + printf("%u (%u) [%c] %u %u bytes %ld.%06ld %ld.%06ld %.3f fps tstamp type/src %s/%s\n", i, buf.index, (buf.flags & V4L2_BUF_FLAG_ERROR) ? 'E' : '-', buf.sequence, buf.bytesused, buf.timestamp.tv_sec, - buf.timestamp.tv_usec, ts.tv_sec, ts.tv_nsec/1000, fps); + buf.timestamp.tv_usec, ts.tv_sec, ts.tv_nsec/1000, fps, + ts_type, ts_source); last = buf.timestamp;
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> --- yavta.c | 52 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 22 deletions(-)