xiaoxiang781216 commented on code in PR #7687:
URL: https://github.com/apache/nuttx/pull/7687#discussion_r1040980742
##########
drivers/video/video.c:
##########
@@ -219,6 +223,7 @@ static int validate_frame_setting(enum v4l2_buf_type type,
uint8_t nr_fmt,
FAR video_format_t *vfmt,
FAR struct v4l2_fract *interval);
+static size_t video_fmt_buf_size(video_format_t *vf);
Review Comment:
```suggestion
static size_t video_fmt_buf_size(FAR video_format_t *vf);
```
##########
drivers/video/video.c:
##########
@@ -1213,6 +1282,8 @@ static int video_qbuf(FAR struct video_mng_s *vmng,
return -ENOMEM;
}
+ buf->length = video_fmt_buf_size(&type_inf->fmt[VIDEO_FMT_MAIN]);
Review Comment:
need check buf->memory == V4L2_MEMORY_MMAP before assignment
##########
drivers/video/video.c:
##########
@@ -1236,13 +1307,27 @@ static int video_qbuf(FAR struct video_mng_s *vmng,
(&type_inf->bufinf);
if (container)
{
- start_capture(buf->type,
- type_inf->nr_fmt,
- type_inf->fmt,
- &type_inf->clip,
- &type_inf->frame_interval,
- container->buf.m.userptr,
- container->buf.length);
+ if (container->buf.memory == V4L2_MEMORY_MMAP)
Review Comment:
why do you need check V4L2_MEMORY_MMAP? userptr already point to the right
place.
##########
drivers/video/video.c:
##########
@@ -1213,6 +1282,8 @@ static int video_qbuf(FAR struct video_mng_s *vmng,
return -ENOMEM;
}
+ buf->length = video_fmt_buf_size(&type_inf->fmt[VIDEO_FMT_MAIN]);
+ buf->m.offset = buf->length * buf->index;
memcpy(&container->buf, buf, sizeof(struct v4l2_buffer));
Review Comment:
let's update container->buf instead buf
##########
drivers/video/video.c:
##########
@@ -1191,7 +1260,7 @@ static int video_qbuf(FAR struct video_mng_s *vmng,
enum video_state_e next_video_state;
irqstate_t flags;
- if ((vmng == NULL) || (buf == NULL))
+ if (vmng == NULL || buf == NULL)
Review Comment:
let's revert the change, I have a patch to update all place
##########
drivers/video/video.c:
##########
@@ -123,6 +126,7 @@ struct video_type_inf_s
struct v4l2_rect clip;
struct v4l2_fract frame_interval;
video_framebuff_t bufinf;
+ FAR void *vmem_heap; /* for V4L2_MEMORY_MMAP buffers */
Review Comment:
```suggestion
FAR uint8_t *vmem_heap; /* for V4L2_MEMORY_MMAP buffers */
```
help to compute the buffer location
##########
drivers/video/video.c:
##########
@@ -1172,17 +1195,63 @@ static int video_reqbufs(FAR struct video_mng_s
*vmng,
}
else
{
+ if (reqbufs->count > CONFIG_VIDEO_REQBUFS_COUNT_MAX)
+ {
+ reqbufs->count = CONFIG_VIDEO_REQBUFS_COUNT_MAX;
+ }
+
video_framebuff_change_mode(&type_inf->bufinf, reqbufs->mode);
ret = video_framebuff_realloc_container(&type_inf->bufinf,
reqbufs->count);
+ if ((ret == OK) && (reqbufs->memory == V4L2_MEMORY_MMAP))
+ {
+ if (type_inf->vmem_heap != NULL)
+ {
+ kmm_free(type_inf->vmem_heap);
+ }
+
+ type_inf->vmem_heap = kmm_memalign(32, reqbufs->count *
Review Comment:
```suggestion
type_inf->vmem_heap = kumm_memalign(32, reqbufs->count *
```
##########
drivers/video/video.c:
##########
@@ -219,6 +223,7 @@ static int validate_frame_setting(enum v4l2_buf_type type,
uint8_t nr_fmt,
FAR video_format_t *vfmt,
FAR struct v4l2_fract *interval);
+static size_t video_fmt_buf_size(video_format_t *vf);
Review Comment:
video_fmt_buf_size->get_bufsize to align with is_bufsize_sufficient
##########
drivers/video/video.c:
##########
@@ -1151,7 +1174,7 @@ static int video_reqbufs(FAR struct video_mng_s
*vmng,
FAR video_type_inf_t *type_inf;
irqstate_t flags;
- if ((vmng == NULL) || (reqbufs == NULL))
+ if (vmng == NULL || reqbufs == NULL)
Review Comment:
let's revert the change, I have a patch to update all place
##########
drivers/video/video.c:
##########
@@ -3331,8 +3444,18 @@ static int video_complete_capture(uint8_t err_code,
uint32_t datasize)
}
else
{
- g_video_data_ops->set_buf((FAR uint8_t *)container->buf.m.userptr,
- container->buf.length);
+ if (container->buf.memory == V4L2_MEMORY_MMAP)
Review Comment:
why do you need check V4L2_MEMORY_MMAP? userptr already point to the right
place.
##########
drivers/video/video.c:
##########
@@ -1172,17 +1195,63 @@ static int video_reqbufs(FAR struct video_mng_s
*vmng,
}
else
{
+ if (reqbufs->count > CONFIG_VIDEO_REQBUFS_COUNT_MAX)
+ {
+ reqbufs->count = CONFIG_VIDEO_REQBUFS_COUNT_MAX;
+ }
+
video_framebuff_change_mode(&type_inf->bufinf, reqbufs->mode);
ret = video_framebuff_realloc_container(&type_inf->bufinf,
reqbufs->count);
+ if ((ret == OK) && (reqbufs->memory == V4L2_MEMORY_MMAP))
Review Comment:
```suggestion
if (ret == OK && reqbufs->memory == V4L2_MEMORY_MMAP)
```
##########
drivers/video/video.c:
##########
@@ -1172,17 +1195,63 @@ static int video_reqbufs(FAR struct video_mng_s
*vmng,
}
else
{
+ if (reqbufs->count > CONFIG_VIDEO_REQBUFS_COUNT_MAX)
+ {
+ reqbufs->count = CONFIG_VIDEO_REQBUFS_COUNT_MAX;
+ }
+
video_framebuff_change_mode(&type_inf->bufinf, reqbufs->mode);
ret = video_framebuff_realloc_container(&type_inf->bufinf,
Review Comment:
let's remove the count check in video_framebuff_realloc_container and
V4L2_REQBUFS_COUNT_MAX
##########
drivers/video/video.c:
##########
@@ -940,6 +958,11 @@ static void cleanup_streamresources(FAR video_type_inf_t
*type_inf)
video_framebuff_uninit(&type_inf->bufinf);
nxsem_destroy(&type_inf->wait_capture.dqbuf_wait_flg);
nxmutex_destroy(&type_inf->lock_state);
+ if (type_inf->vmem_heap != NULL)
+ {
+ kmm_free(type_inf->vmem_heap);
Review Comment:
```suggestion
kumm_free(type_inf->vmem_heap);
```
##########
drivers/video/video.c:
##########
@@ -1757,7 +1858,7 @@ static int video_streamon(FAR struct video_mng_s *vmng,
enum video_state_e next_video_state;
int ret = OK;
- if ((vmng == NULL) || (type == NULL))
+ if (vmng == NULL || type == NULL)
Review Comment:
let's revert the change, I have a patch to update all place
##########
drivers/video/video.c:
##########
@@ -705,13 +710,26 @@ static void change_video_state(FAR video_mng_t *vmng,
video_framebuff_get_vacant_container(&vmng->video_inf.bufinf);
if (container)
{
- start_capture(V4L2_BUF_TYPE_VIDEO_CAPTURE,
- vmng->video_inf.nr_fmt,
- vmng->video_inf.fmt,
- &vmng->video_inf.clip,
- &vmng->video_inf.frame_interval,
- container->buf.m.userptr,
- container->buf.length);
+ if (container->buf.memory == V4L2_MEMORY_MMAP)
Review Comment:
why do you need check V4L2_MEMORY_MMAP? userptr already point to the right
place.
##########
drivers/video/video.c:
##########
@@ -1172,17 +1195,63 @@ static int video_reqbufs(FAR struct video_mng_s
*vmng,
}
else
{
+ if (reqbufs->count > CONFIG_VIDEO_REQBUFS_COUNT_MAX)
+ {
+ reqbufs->count = CONFIG_VIDEO_REQBUFS_COUNT_MAX;
+ }
+
video_framebuff_change_mode(&type_inf->bufinf, reqbufs->mode);
ret = video_framebuff_realloc_container(&type_inf->bufinf,
reqbufs->count);
+ if ((ret == OK) && (reqbufs->memory == V4L2_MEMORY_MMAP))
+ {
+ if (type_inf->vmem_heap != NULL)
+ {
+ kmm_free(type_inf->vmem_heap);
Review Comment:
```suggestion
kumm_free(type_inf->vmem_heap);
```
##########
drivers/video/video.c:
##########
@@ -1800,7 +1901,7 @@ static int video_streamoff(FAR struct video_mng_s *vmng,
irqstate_t flags;
int ret = OK;
- if ((vmng == NULL) || (type == NULL))
+ if (vmng == NULL || type == NULL)
Review Comment:
let's revert the change, I have a patch to update all place
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
