| Message ID | 1418729778-14480-1-git-send-email-k.debski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kamil, 2014-12-16 12:36 GMT+01:00 Kamil Debski <k.debski@samsung.com>: > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the behavior > of __fill_vb2_buffer function, so that if bytesused is 0 it is set to the > size of the buffer. However, bytesused set to 0 is used by older codec > drivers as as indication used to mark the end of stream. > > To keep backward compatibility, this patch adds a flag passed to the > vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the flag is > set upon initialization of the queue, the videobuf2 keeps the value of > bytesused intact and passes it to the driver. Nice, this is something we were planning to do :). But I would split this patch and the second which is specific to s5p-mfc as this is core specific and should be independant. > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Signed-off-by: Kamil Debski <k.debski@samsung.com> > --- > drivers/media/v4l2-core/videobuf2-core.c | 33 ++++++++++++++++++++++++------ > include/media/videobuf2-core.h | 3 +++ > 2 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index d09a891..1068dbb 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -1276,13 +1276,23 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * userspace clearly never bothered to set it and > * it's a safe assumption that they really meant to > * use the full plane sizes. > + * > + * Some drivers, e.g. old codec drivers, use bytesused > + * == 0 as a way to indicate that streaming is finished. > + * In that case, the driver should use the following > + * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old > + * userspace applications working. Not sure if this comment is necessary, as this is already in the commit ? > */ > for (plane = 0; plane < vb->num_planes; ++plane) { > struct v4l2_plane *pdst = &v4l2_planes[plane]; > struct v4l2_plane *psrc = &b->m.planes[plane]; > > - pdst->bytesused = psrc->bytesused ? > - psrc->bytesused : pdst->length; > + if (vb->vb2_queue->io_flags & > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) > + pdst->bytesused = psrc->bytesused; > + else > + pdst->bytesused = psrc->bytesused ? > + psrc->bytesused : pdst->length; > pdst->data_offset = psrc->data_offset; > } > } > @@ -1295,6 +1305,12 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * > * If bytesused == 0 for the output buffer, then fall back > * to the full buffer size as that's a sensible default. > + * > + * Some drivers, e.g. old codec drivers, use bytesused == 0 > + * as a way to indicate that streaming is finished. In that > + * case, the driver should use the following io_flag > + * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace > + * applications working. Again, not sure this is useful. > */ > if (b->memory == V4L2_MEMORY_USERPTR) { > v4l2_planes[0].m.userptr = b->m.userptr; > @@ -1306,11 +1322,16 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > v4l2_planes[0].length = b->length; > } > > - if (V4L2_TYPE_IS_OUTPUT(b->type)) > - v4l2_planes[0].bytesused = b->bytesused ? > - b->bytesused : v4l2_planes[0].length; > - else > + if (V4L2_TYPE_IS_OUTPUT(b->type)) { > + if (vb->vb2_queue->io_flags & > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) > + v4l2_planes[0].bytesused = b->bytesused; > + else > + v4l2_planes[0].bytesused = b->bytesused ? > + b->bytesused : v4l2_planes[0].length; > + } else { > v4l2_planes[0].bytesused = 0; > + } > > } > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index bd2cec2..0540bc3 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -138,10 +138,13 @@ enum vb2_io_modes { > * by default the 'streaming' style is used by the file io emulator > * @VB2_FILEIO_READ_ONCE: report EOF after reading the first buffer > * @VB2_FILEIO_WRITE_IMMEDIATELY: queue buffer after each write() call > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED: the driver setting this flag will handle > + * bytesused == 0 as a special case > */ > enum vb2_fileio_flags { > VB2_FILEIO_READ_ONCE = (1 << 0), > VB2_FILEIO_WRITE_IMMEDIATELY = (1 << 1), > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2), > }; > > /** > -- > 1.7.9.5 > > -- > 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 I tested it with your coda patch too on i.MX6, thank you, this was annoying :). JM -- 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 Jean, > From: Jean-Michel Hautbois [mailto:jhautbois@gmail.com] > Sent: Friday, December 19, 2014 3:36 PM > To: Kamil Debski > Cc: Linux Media Mailing List; m.szyprowski@samsung.com; Hans Verkuil; > Nicolas Dufresne > Subject: Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag > to vb2_fileio_flags > > Hi Kamil, > > 2014-12-16 12:36 GMT+01:00 Kamil Debski <k.debski@samsung.com>: > > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the > > behavior of __fill_vb2_buffer function, so that if bytesused is 0 it > > is set to the size of the buffer. However, bytesused set to 0 is used > > by older codec drivers as as indication used to mark the end of > stream. > > > > To keep backward compatibility, this patch adds a flag passed to the > > vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the > flag > > is set upon initialization of the queue, the videobuf2 keeps the > value > > of bytesused intact and passes it to the driver. > > Nice, this is something we were planning to do :). > But I would split this patch and the second which is specific to s5p- > mfc as this is core specific and should be independant. This patch contains only changes in the videobuf2-core.c file. The next patch in the series contains changes in the s5p-mfc driver. There is another patch sent today that adds this flag to coda. These are all separate patches, two of them are in a single patchset. Actually, I would send all of them in one patchset, but initially I missed that the coda driver should also have this change applied. (Nicolas, thank you for reminding me to do this on IRC). > > > > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > > --- > > drivers/media/v4l2-core/videobuf2-core.c | 33 > ++++++++++++++++++++++++------ > > include/media/videobuf2-core.h | 3 +++ > > 2 files changed, 30 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > b/drivers/media/v4l2-core/videobuf2-core.c > > index d09a891..1068dbb 100644 > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > @@ -1276,13 +1276,23 @@ static void __fill_vb2_buffer(struct > vb2_buffer *vb, const struct v4l2_buffer *b > > * userspace clearly never bothered to set it > and > > * it's a safe assumption that they really > meant to > > * use the full plane sizes. > > + * > > + * Some drivers, e.g. old codec drivers, use > bytesused > > + * == 0 as a way to indicate that streaming > is finished. > > + * In that case, the driver should use the > following > > + * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to > keep old > > + * userspace applications working. > > Not sure if this comment is necessary, as this is already in the > commit ? The comment were present in the file already, I expanded them to cover the exception when the VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag is set. It is also explained in the commit, I agree, but in the end one usually looks in the source code. > > > */ > > for (plane = 0; plane < vb->num_planes; > ++plane) { > > struct v4l2_plane *pdst = > &v4l2_planes[plane]; > > struct v4l2_plane *psrc = > > &b->m.planes[plane]; > > > > - pdst->bytesused = psrc->bytesused ? > > - psrc->bytesused : pdst- > >length; > > + if (vb->vb2_queue->io_flags & > > + > VB2_FILEIO_ALLOW_ZERO_BYTESUSED) > > + pdst->bytesused = psrc- > >bytesused; > > + else > > + pdst->bytesused = psrc- > >bytesused ? > > + psrc->bytesused : > > + pdst->length; > > pdst->data_offset = psrc->data_offset; > > } > > } > > @@ -1295,6 +1305,12 @@ static void __fill_vb2_buffer(struct > vb2_buffer *vb, const struct v4l2_buffer *b > > * > > * If bytesused == 0 for the output buffer, then fall > back > > * to the full buffer size as that's a sensible > default. > > + * > > + * Some drivers, e.g. old codec drivers, use > bytesused == 0 > > + * as a way to indicate that streaming is finished. > In that > > + * case, the driver should use the following io_flag > > + * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old > userspace > > + * applications working. > > Again, not sure this is useful. Same applies here, I expanded the comment to cover a new case. > > > */ > > if (b->memory == V4L2_MEMORY_USERPTR) { > > v4l2_planes[0].m.userptr = b->m.userptr; @@ > > -1306,11 +1322,16 @@ static void __fill_vb2_buffer(struct vb2_buffer > *vb, const struct v4l2_buffer *b > > v4l2_planes[0].length = b->length; > > } > > > > - if (V4L2_TYPE_IS_OUTPUT(b->type)) > > - v4l2_planes[0].bytesused = b->bytesused ? > > - b->bytesused : v4l2_planes[0].length; > > - else > > + if (V4L2_TYPE_IS_OUTPUT(b->type)) { > > + if (vb->vb2_queue->io_flags & > > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) > > + v4l2_planes[0].bytesused = b- > >bytesused; > > + else > > + v4l2_planes[0].bytesused = b- > >bytesused ? > > + b->bytesused : > v4l2_planes[0].length; > > + } else { > > v4l2_planes[0].bytesused = 0; > > + } > > > > } > > > > diff --git a/include/media/videobuf2-core.h > > b/include/media/videobuf2-core.h index bd2cec2..0540bc3 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -138,10 +138,13 @@ enum vb2_io_modes { > > * by default the 'streaming' style is used by the file io emulator > > * @VB2_FILEIO_READ_ONCE: report EOF after reading the first > buffer > > * @VB2_FILEIO_WRITE_IMMEDIATELY: queue buffer after each > write() call > > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED: the driver setting this flag > will handle > > + * bytesused == 0 as a special > case > > */ > > enum vb2_fileio_flags { > > VB2_FILEIO_READ_ONCE = (1 << 0), > > VB2_FILEIO_WRITE_IMMEDIATELY = (1 << 1), > > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2), > > }; > > > > /** > > -- > > 1.7.9.5 > > > > -- > > 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 > > I tested it with your coda patch too on i.MX6, thank you, this was > annoying :). > JM You're welcome. Thanks for testing :) Best wishes,
2014-12-19 17:03 GMT+01:00 Kamil Debski <k.debski@samsung.com>: > Hi Jean, > >> From: Jean-Michel Hautbois [mailto:jhautbois@gmail.com] >> Sent: Friday, December 19, 2014 3:36 PM >> To: Kamil Debski >> Cc: Linux Media Mailing List; m.szyprowski@samsung.com; Hans Verkuil; >> Nicolas Dufresne >> Subject: Re: [PATCH 1/2] vb2: Add VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag >> to vb2_fileio_flags >> >> Hi Kamil, >> >> 2014-12-16 12:36 GMT+01:00 Kamil Debski <k.debski@samsung.com>: >> > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the >> > behavior of __fill_vb2_buffer function, so that if bytesused is 0 it >> > is set to the size of the buffer. However, bytesused set to 0 is used >> > by older codec drivers as as indication used to mark the end of >> stream. >> > >> > To keep backward compatibility, this patch adds a flag passed to the >> > vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the >> flag >> > is set upon initialization of the queue, the videobuf2 keeps the >> value >> > of bytesused intact and passes it to the driver. >> >> Nice, this is something we were planning to do :). >> But I would split this patch and the second which is specific to s5p- >> mfc as this is core specific and should be independant. > > This patch contains only changes in the videobuf2-core.c file. The next > patch in the series contains changes in the s5p-mfc driver. There is > another patch sent today that adds this flag to coda. > > These are all separate patches, two of them are in a single patchset. > Actually, I would send all of them in one patchset, but initially I missed > that the coda driver should also have this change applied. (Nicolas, thank > you for reminding me to do this on IRC). OK, This is why I have been confused. Well, I still think that core modification should be a separate patch, and maybe s5f-mpc and coda be in the same patchset. Not a problem. >> >> >> > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> >> > Signed-off-by: Kamil Debski <k.debski@samsung.com> >> > --- >> > drivers/media/v4l2-core/videobuf2-core.c | 33 >> ++++++++++++++++++++++++------ >> > include/media/videobuf2-core.h | 3 +++ >> > 2 files changed, 30 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c >> > b/drivers/media/v4l2-core/videobuf2-core.c >> > index d09a891..1068dbb 100644 >> > --- a/drivers/media/v4l2-core/videobuf2-core.c >> > +++ b/drivers/media/v4l2-core/videobuf2-core.c >> > @@ -1276,13 +1276,23 @@ static void __fill_vb2_buffer(struct >> vb2_buffer *vb, const struct v4l2_buffer *b >> > * userspace clearly never bothered to set it >> and >> > * it's a safe assumption that they really >> meant to >> > * use the full plane sizes. >> > + * >> > + * Some drivers, e.g. old codec drivers, use >> bytesused >> > + * == 0 as a way to indicate that streaming >> is finished. >> > + * In that case, the driver should use the >> following >> > + * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to >> keep old >> > + * userspace applications working. >> >> Not sure if this comment is necessary, as this is already in the >> commit ? > > The comment were present in the file already, I expanded them to cover the > exception when the VB2_FILEIO_ALLOW_ZERO_BYTESUSED flag is set. > It is also explained in the commit, I agree, but in the end one usually > looks in the source code. OK >> >> > */ >> > for (plane = 0; plane < vb->num_planes; >> ++plane) { >> > struct v4l2_plane *pdst = >> &v4l2_planes[plane]; >> > struct v4l2_plane *psrc = >> > &b->m.planes[plane]; >> > >> > - pdst->bytesused = psrc->bytesused ? >> > - psrc->bytesused : pdst- >> >length; >> > + if (vb->vb2_queue->io_flags & >> > + >> VB2_FILEIO_ALLOW_ZERO_BYTESUSED) >> > + pdst->bytesused = psrc- >> >bytesused; >> > + else >> > + pdst->bytesused = psrc- >> >bytesused ? >> > + psrc->bytesused : >> > + pdst->length; >> > pdst->data_offset = psrc->data_offset; >> > } >> > } >> > @@ -1295,6 +1305,12 @@ static void __fill_vb2_buffer(struct >> vb2_buffer *vb, const struct v4l2_buffer *b >> > * >> > * If bytesused == 0 for the output buffer, then fall >> back >> > * to the full buffer size as that's a sensible >> default. >> > + * >> > + * Some drivers, e.g. old codec drivers, use >> bytesused == 0 >> > + * as a way to indicate that streaming is finished. >> In that >> > + * case, the driver should use the following io_flag >> > + * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old >> userspace >> > + * applications working. >> >> Again, not sure this is useful. > > Same applies here, I expanded the comment to cover a new case. > >> >> > */ >> > if (b->memory == V4L2_MEMORY_USERPTR) { >> > v4l2_planes[0].m.userptr = b->m.userptr; @@ >> > -1306,11 +1322,16 @@ static void __fill_vb2_buffer(struct vb2_buffer >> *vb, const struct v4l2_buffer *b >> > v4l2_planes[0].length = b->length; >> > } >> > >> > - if (V4L2_TYPE_IS_OUTPUT(b->type)) >> > - v4l2_planes[0].bytesused = b->bytesused ? >> > - b->bytesused : v4l2_planes[0].length; >> > - else >> > + if (V4L2_TYPE_IS_OUTPUT(b->type)) { >> > + if (vb->vb2_queue->io_flags & >> > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) >> > + v4l2_planes[0].bytesused = b- >> >bytesused; >> > + else >> > + v4l2_planes[0].bytesused = b- >> >bytesused ? >> > + b->bytesused : >> v4l2_planes[0].length; >> > + } else { >> > v4l2_planes[0].bytesused = 0; >> > + } >> > >> > } >> > >> > diff --git a/include/media/videobuf2-core.h >> > b/include/media/videobuf2-core.h index bd2cec2..0540bc3 100644 >> > --- a/include/media/videobuf2-core.h >> > +++ b/include/media/videobuf2-core.h >> > @@ -138,10 +138,13 @@ enum vb2_io_modes { >> > * by default the 'streaming' style is used by the file io emulator >> > * @VB2_FILEIO_READ_ONCE: report EOF after reading the first >> buffer >> > * @VB2_FILEIO_WRITE_IMMEDIATELY: queue buffer after each >> write() call >> > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED: the driver setting this flag >> will handle >> > + * bytesused == 0 as a special >> case >> > */ >> > enum vb2_fileio_flags { >> > VB2_FILEIO_READ_ONCE = (1 << 0), >> > VB2_FILEIO_WRITE_IMMEDIATELY = (1 << 1), >> > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2), >> > }; >> > >> > /** >> > -- >> > 1.7.9.5 >> > >> > -- >> > 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 >> >> I tested it with your coda patch too on i.MX6, thank you, this was >> annoying :). >> JM > > You're welcome. Thanks for testing :) > > Best wishes, Thanks, and best wishes too. JM -- 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 Kamil, On 12/16/14 12:36, Kamil Debski wrote: > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the behavior > of __fill_vb2_buffer function, so that if bytesused is 0 it is set to the > size of the buffer. However, bytesused set to 0 is used by older codec > drivers as as indication used to mark the end of stream. > > To keep backward compatibility, this patch adds a flag passed to the > vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the flag is > set upon initialization of the queue, the videobuf2 keeps the value of > bytesused intact and passes it to the driver. What is the status of this patch series? Note that io_flags is really the wrong place for this flag, it should be io_modes. This flag has nothing to do with file I/O. Regards, Hans > > Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Signed-off-by: Kamil Debski <k.debski@samsung.com> > --- > drivers/media/v4l2-core/videobuf2-core.c | 33 ++++++++++++++++++++++++------ > include/media/videobuf2-core.h | 3 +++ > 2 files changed, 30 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index d09a891..1068dbb 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -1276,13 +1276,23 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * userspace clearly never bothered to set it and > * it's a safe assumption that they really meant to > * use the full plane sizes. > + * > + * Some drivers, e.g. old codec drivers, use bytesused > + * == 0 as a way to indicate that streaming is finished. > + * In that case, the driver should use the following > + * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old > + * userspace applications working. > */ > for (plane = 0; plane < vb->num_planes; ++plane) { > struct v4l2_plane *pdst = &v4l2_planes[plane]; > struct v4l2_plane *psrc = &b->m.planes[plane]; > > - pdst->bytesused = psrc->bytesused ? > - psrc->bytesused : pdst->length; > + if (vb->vb2_queue->io_flags & > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) > + pdst->bytesused = psrc->bytesused; > + else > + pdst->bytesused = psrc->bytesused ? > + psrc->bytesused : pdst->length; > pdst->data_offset = psrc->data_offset; > } > } > @@ -1295,6 +1305,12 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * > * If bytesused == 0 for the output buffer, then fall back > * to the full buffer size as that's a sensible default. > + * > + * Some drivers, e.g. old codec drivers, use bytesused == 0 > + * as a way to indicate that streaming is finished. In that > + * case, the driver should use the following io_flag > + * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace > + * applications working. > */ > if (b->memory == V4L2_MEMORY_USERPTR) { > v4l2_planes[0].m.userptr = b->m.userptr; > @@ -1306,11 +1322,16 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > v4l2_planes[0].length = b->length; > } > > - if (V4L2_TYPE_IS_OUTPUT(b->type)) > - v4l2_planes[0].bytesused = b->bytesused ? > - b->bytesused : v4l2_planes[0].length; > - else > + if (V4L2_TYPE_IS_OUTPUT(b->type)) { > + if (vb->vb2_queue->io_flags & > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) > + v4l2_planes[0].bytesused = b->bytesused; > + else > + v4l2_planes[0].bytesused = b->bytesused ? > + b->bytesused : v4l2_planes[0].length; > + } else { > v4l2_planes[0].bytesused = 0; > + } > > } > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index bd2cec2..0540bc3 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -138,10 +138,13 @@ enum vb2_io_modes { > * by default the 'streaming' style is used by the file io emulator > * @VB2_FILEIO_READ_ONCE: report EOF after reading the first buffer > * @VB2_FILEIO_WRITE_IMMEDIATELY: queue buffer after each write() call > + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED: the driver setting this flag will handle > + * bytesused == 0 as a special case > */ > enum vb2_fileio_flags { > VB2_FILEIO_READ_ONCE = (1 << 0), > VB2_FILEIO_WRITE_IMMEDIATELY = (1 << 1), > + VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2), > }; > > /** > -- 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, > From: Hans Verkuil [mailto:hverkuil@xs4all.nl] > Sent: Tuesday, February 17, 2015 10:11 AM > > Hi Kamil, > > On 12/16/14 12:36, Kamil Debski wrote: > > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the > > behavior of __fill_vb2_buffer function, so that if bytesused is 0 it > > is set to the size of the buffer. However, bytesused set to 0 is used > > by older codec drivers as as indication used to mark the end of > stream. > > > > To keep backward compatibility, this patch adds a flag passed to the > > vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the > flag > > is set upon initialization of the queue, the videobuf2 keeps the > value > > of bytesused intact and passes it to the driver. > > What is the status of this patch series? I have to admit that I had forgotten a bit about this patch, because of other important work. Thanks for reminding me :) > Note that io_flags is really the wrong place for this flag, it should > be io_modes. This flag has nothing to do with file I/O. What do you think about adding a separate flags field into the vb2_queue structure? This could be combined with changing io_flags to u8 or a bit field to save space. Best wishes,
On 02/18/15 10:42, Kamil Debski wrote: > Hi Hans, > >> From: Hans Verkuil [mailto:hverkuil@xs4all.nl] >> Sent: Tuesday, February 17, 2015 10:11 AM >> >> Hi Kamil, >> >> On 12/16/14 12:36, Kamil Debski wrote: >>> The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the >>> behavior of __fill_vb2_buffer function, so that if bytesused is 0 it >>> is set to the size of the buffer. However, bytesused set to 0 is used >>> by older codec drivers as as indication used to mark the end of >> stream. >>> >>> To keep backward compatibility, this patch adds a flag passed to the >>> vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the >> flag >>> is set upon initialization of the queue, the videobuf2 keeps the >> value >>> of bytesused intact and passes it to the driver. >> >> What is the status of this patch series? > > I have to admit that I had forgotten a bit about this patch, because of > other > important work. Thanks for reminding me :) > >> Note that io_flags is really the wrong place for this flag, it should >> be io_modes. This flag has nothing to do with file I/O. > > What do you think about adding a separate flags field into the vb2_queue > structure? This could be combined with changing io_flags to u8 or a bit > field > to save space. I think changing io_flags to a bitfield is a good idea. unsigned fileio_read_once:1; unsigned fileio_write_immediately:1; unsigned allow_zero_bytesused:1; However, going back to allow_zero_bytesused: this has been broken for quite some time without anyone complaining (other than you :-) ). Might it not be better to just fix this properly by calling V4L2_DEC_CMD_STOP, as done here: https://www.mail-archive.com/linux-media@vger.kernel.org/msg84916.html, and drop the support for zero bytesused to mark EOS entirely? I might be too optimistic here. Or perhaps at least add a pr_warn telling users to switch to V4L2_DEC_CMD_STOP since this will be removed in 2017 or whatever. 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
Hi, > From: Hans Verkuil [mailto:hverkuil@xs4all.nl] > Sent: Wednesday, February 18, 2015 10:58 AM > > On 02/18/15 10:42, Kamil Debski wrote: > > Hi Hans, > > > >> From: Hans Verkuil [mailto:hverkuil@xs4all.nl] > >> Sent: Tuesday, February 17, 2015 10:11 AM > >> > >> Hi Kamil, > >> > >> On 12/16/14 12:36, Kamil Debski wrote: > >>> The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the > >>> behavior of __fill_vb2_buffer function, so that if bytesused is 0 > it > >>> is set to the size of the buffer. However, bytesused set to 0 is > >>> used by older codec drivers as as indication used to mark the end > of > >> stream. > >>> > >>> To keep backward compatibility, this patch adds a flag passed to > the > >>> vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the > >> flag > >>> is set upon initialization of the queue, the videobuf2 keeps the > >> value > >>> of bytesused intact and passes it to the driver. > >> > >> What is the status of this patch series? > > > > I have to admit that I had forgotten a bit about this patch, because > > of other important work. Thanks for reminding me :) > > > >> Note that io_flags is really the wrong place for this flag, it > should > >> be io_modes. This flag has nothing to do with file I/O. > > > > What do you think about adding a separate flags field into the > > vb2_queue structure? This could be combined with changing io_flags to > > u8 or a bit field to save space. > > I think changing io_flags to a bitfield is a good idea. > > unsigned fileio_read_once:1; > unsigned fileio_write_immediately:1; > unsigned allow_zero_bytesused:1; > > However, going back to allow_zero_bytesused: this has been broken for > quite some time without anyone complaining (other than you :-) ). If I remember correctly, it was Nicolas who reported to me the problem on the IRC. > Might > it not be better to just fix this properly by calling > V4L2_DEC_CMD_STOP, as done here: https://www.mail-archive.com/linux- > media@vger.kernel.org/msg84916.html, > and drop the support for zero bytesused to mark EOS entirely? I think it would be good to have the backward compatibility for some time. > I might be too optimistic here. Or perhaps at least add a pr_warn > telling users to switch to V4L2_DEC_CMD_STOP since this will be removed > in 2017 or whatever. Where do you see the pr_warn? I guess it would be good if it was only displayed once and only when the app uses bytesused == 0 to signal the EOS. Do you think alike? > > Regards, > > Hans Best wishes,
On 02/18/15 11:31, Kamil Debski wrote: > Hi, > >> From: Hans Verkuil [mailto:hverkuil@xs4all.nl] >> Sent: Wednesday, February 18, 2015 10:58 AM >> >> On 02/18/15 10:42, Kamil Debski wrote: >>> Hi Hans, >>> >>>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl] >>>> Sent: Tuesday, February 17, 2015 10:11 AM >>>> >>>> Hi Kamil, >>>> >>>> On 12/16/14 12:36, Kamil Debski wrote: >>>>> The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the >>>>> behavior of __fill_vb2_buffer function, so that if bytesused is 0 >> it >>>>> is set to the size of the buffer. However, bytesused set to 0 is >>>>> used by older codec drivers as as indication used to mark the end >> of >>>> stream. >>>>> >>>>> To keep backward compatibility, this patch adds a flag passed to >> the >>>>> vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the >>>> flag >>>>> is set upon initialization of the queue, the videobuf2 keeps the >>>> value >>>>> of bytesused intact and passes it to the driver. >>>> >>>> What is the status of this patch series? >>> >>> I have to admit that I had forgotten a bit about this patch, because >>> of other important work. Thanks for reminding me :) >>> >>>> Note that io_flags is really the wrong place for this flag, it >> should >>>> be io_modes. This flag has nothing to do with file I/O. >>> >>> What do you think about adding a separate flags field into the >>> vb2_queue structure? This could be combined with changing io_flags to >>> u8 or a bit field to save space. >> >> I think changing io_flags to a bitfield is a good idea. >> >> unsigned fileio_read_once:1; >> unsigned fileio_write_immediately:1; >> unsigned allow_zero_bytesused:1; >> >> However, going back to allow_zero_bytesused: this has been broken for >> quite some time without anyone complaining (other than you :-) ). > > If I remember correctly, it was Nicolas who reported to me the problem > on the IRC. > >> Might >> it not be better to just fix this properly by calling >> V4L2_DEC_CMD_STOP, as done here: https://www.mail-archive.com/linux- >> media@vger.kernel.org/msg84916.html, >> and drop the support for zero bytesused to mark EOS entirely? > > I think it would be good to have the backward compatibility for some time. > >> I might be too optimistic here. Or perhaps at least add a pr_warn >> telling users to switch to V4L2_DEC_CMD_STOP since this will be removed >> in 2017 or whatever. > > Where do you see the pr_warn? I guess it would be good if it was only > displayed once and only when the app uses bytesused == 0 to signal the > EOS. Do you think alike? Right, pr_warn_once() would do that. As I discussed with Pawel on irc I also think we need to do a pr_warn_once if the application doesn't set bytesused for output streams. Right now we replace bytesused by length to cater for bad apps, but I really hate that. I'd like to eventually require a proper setting for bytesused, but that means apps need to be informed that they do something wrong. Unfortunately, other than gstreamer I would expect that all those apps are closed source running on embedded systems. Nicolas, can you confirm that gstreamer is filling in bytesused for output buffers correctly? (I.e., doesn't leave it to 0). I may have mentioned this before, but have you run gstreamer with valgrind? valgrind 3.10 now supports all v4l2 ioctls. Regards, Hans > > >> >> Regards, >> >> Hans > > Best wishes, > -- 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 d09a891..1068dbb 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -1276,13 +1276,23 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b * userspace clearly never bothered to set it and * it's a safe assumption that they really meant to * use the full plane sizes. + * + * Some drivers, e.g. old codec drivers, use bytesused + * == 0 as a way to indicate that streaming is finished. + * In that case, the driver should use the following + * io_flag VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old + * userspace applications working. */ for (plane = 0; plane < vb->num_planes; ++plane) { struct v4l2_plane *pdst = &v4l2_planes[plane]; struct v4l2_plane *psrc = &b->m.planes[plane]; - pdst->bytesused = psrc->bytesused ? - psrc->bytesused : pdst->length; + if (vb->vb2_queue->io_flags & + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) + pdst->bytesused = psrc->bytesused; + else + pdst->bytesused = psrc->bytesused ? + psrc->bytesused : pdst->length; pdst->data_offset = psrc->data_offset; } } @@ -1295,6 +1305,12 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b * * If bytesused == 0 for the output buffer, then fall back * to the full buffer size as that's a sensible default. + * + * Some drivers, e.g. old codec drivers, use bytesused == 0 + * as a way to indicate that streaming is finished. In that + * case, the driver should use the following io_flag + * VB2_FILEIO_ALLOW_ZERO_BYTESUSED to keep old userspace + * applications working. */ if (b->memory == V4L2_MEMORY_USERPTR) { v4l2_planes[0].m.userptr = b->m.userptr; @@ -1306,11 +1322,16 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b v4l2_planes[0].length = b->length; } - if (V4L2_TYPE_IS_OUTPUT(b->type)) - v4l2_planes[0].bytesused = b->bytesused ? - b->bytesused : v4l2_planes[0].length; - else + if (V4L2_TYPE_IS_OUTPUT(b->type)) { + if (vb->vb2_queue->io_flags & + VB2_FILEIO_ALLOW_ZERO_BYTESUSED) + v4l2_planes[0].bytesused = b->bytesused; + else + v4l2_planes[0].bytesused = b->bytesused ? + b->bytesused : v4l2_planes[0].length; + } else { v4l2_planes[0].bytesused = 0; + } } diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index bd2cec2..0540bc3 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -138,10 +138,13 @@ enum vb2_io_modes { * by default the 'streaming' style is used by the file io emulator * @VB2_FILEIO_READ_ONCE: report EOF after reading the first buffer * @VB2_FILEIO_WRITE_IMMEDIATELY: queue buffer after each write() call + * @VB2_FILEIO_ALLOW_ZERO_BYTESUSED: the driver setting this flag will handle + * bytesused == 0 as a special case */ enum vb2_fileio_flags { VB2_FILEIO_READ_ONCE = (1 << 0), VB2_FILEIO_WRITE_IMMEDIATELY = (1 << 1), + VB2_FILEIO_ALLOW_ZERO_BYTESUSED = (1 << 2), }; /**
The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the behavior of __fill_vb2_buffer function, so that if bytesused is 0 it is set to the size of the buffer. However, bytesused set to 0 is used by older codec drivers as as indication used to mark the end of stream. To keep backward compatibility, this patch adds a flag passed to the vb2_queue_init function - VB2_FILEIO_ALLOW_ZERO_BYTESUSED. If the flag is set upon initialization of the queue, the videobuf2 keeps the value of bytesused intact and passes it to the driver. Reported-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Signed-off-by: Kamil Debski <k.debski@samsung.com> --- drivers/media/v4l2-core/videobuf2-core.c | 33 ++++++++++++++++++++++++------ include/media/videobuf2-core.h | 3 +++ 2 files changed, 30 insertions(+), 6 deletions(-)