Re: regarding commit "v4l2-ctl: get fmt after source change" in v4l-utils repository

2019-07-10 Thread Hans Verkuil
On 7/10/19 12:12 PM, Hans Verkuil wrote:
> Hi Dafna,
> 
> On 7/1/19 12:30 PM, Dafna Hirschfeld wrote:
>> commit 84219e2b5d013709ee5259621715966c46eec746
>> Author: Hans Verkuil 
>> Date:   Sat Mar 30 14:16:25 2019 +0100
>>
>> v4l2-ctl: get fmt after source change
>>
>> When a source change event arrives during decoding get the new
>> format at that point instead of after restarting streaming.
>>
>> If there is another source change queued up, then when you call
>> streamon for CAPTURE again it might send the new source change
>> event and update the format for that one, so reading the format
>> after streamon might give the wrong format.
>>
>> Signed-off-by: Hans Verkuil 
>>
>> diff --git a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
>> b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
>> index bb656584..3695a027 100644
>> --- a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
>> +++ b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
>> @@ -2363,12 +2363,11 @@ static void stateful_m2m(cv4l_fd &fd,
>> cv4l_queue &in, cv4l_queue &out,
>> if (in_source_change_event) {
>> in_source_change_event = false;
>> last_buffer = false;
>> +   fd.g_fmt(fmt_in, in.g_type());
>> if (capture_setup(fd, in, exp_fd_p))
>> return;
>> fps_ts[CAP].reset();
>> fps_ts[OUT].reset();
>> -   fd.g_fmt(fmt_out, out.g_type());
>> -   fd.g_fmt(fmt_in, in.g_type());
>> Removing those lines cause inconsistency when the user send the wanted
>> capture pixel format when decoding with the `v4l2-ctl` command. In
>> this case the value of `-v pixelformat=...` is updated in the kernel
>> with the capture_setup function but it is not updated in the fmt_in
>> variable and so the command will try to save the decoded video in a
>> different format from what is configured in the kernel.
>>
>> cap_streaming = true;
>> } else {
>> break;
>>
>>
>> Dafna
>>
> 
> It looks like you are right. Can you try this v4l2-ctl patch?
> 
> Regards,
> 
>   Hans
> 
> [PATCH] v4l2-ctl: let capture_setup return the updated format
> 
> Just getting the new format after a source change event isn't
> enough in the case that -v option was specified by the user.
> 
> Instead let capture_setup optionally return the new format after
> any '-v' changes are applied.
> 
> Signed-off-by: Hans Verkuil 
> Reported-by: Dafna Hirschfeld 

I pushed this patch since your test with 'blog.sh' now works with this
patch applied.

Regards,

Hans

> ---
> diff --git a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp 
> b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> index 28b2b3b9..470fa766 100644
> --- a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> +++ b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> @@ -2146,7 +2146,7 @@ enum stream_type {
>   OUT,
>  };
> 
> -static int capture_setup(cv4l_fd &fd, cv4l_queue &in, cv4l_fd *exp_fd)
> +static int capture_setup(cv4l_fd &fd, cv4l_queue &in, cv4l_fd *exp_fd, 
> cv4l_fmt *new_fmt = NULL)
>  {
>   if (fd.streamoff(in.g_type())) {
>   fprintf(stderr, "%s: fd.streamoff error\n", __func__);
> @@ -2168,6 +2168,10 @@ static int capture_setup(cv4l_fd &fd, cv4l_queue &in, 
> cv4l_fd *exp_fd)
>   return -1;
>   }
>   fd.s_fmt(fmt, in.g_type());
> + if (new_fmt)
> + *new_fmt = fmt;
> + } else if (new_fmt) {
> + fd.g_fmt(*new_fmt, in.g_type());
>   }
>   get_cap_compose_rect(fd);
> 
> @@ -2367,8 +2371,7 @@ static void stateful_m2m(cv4l_fd &fd, cv4l_queue &in, 
> cv4l_queue &out,
>   if (in_source_change_event) {
>   in_source_change_event = false;
>   last_buffer = false;
> - fd.g_fmt(fmt_in, in.g_type());
> - if (capture_setup(fd, in, exp_fd_p))
> + if (capture_setup(fd, in, exp_fd_p, &fmt_in))
>   return;
>   fps_ts[CAP].reset();
>   fps_ts[OUT].reset();
> 



Re: regarding commit "v4l2-ctl: get fmt after source change" in v4l-utils repository

2019-07-10 Thread Hans Verkuil
Hi Dafna,

On 7/1/19 12:30 PM, Dafna Hirschfeld wrote:
> commit 84219e2b5d013709ee5259621715966c46eec746
> Author: Hans Verkuil 
> Date:   Sat Mar 30 14:16:25 2019 +0100
> 
> v4l2-ctl: get fmt after source change
> 
> When a source change event arrives during decoding get the new
> format at that point instead of after restarting streaming.
> 
> If there is another source change queued up, then when you call
> streamon for CAPTURE again it might send the new source change
> event and update the format for that one, so reading the format
> after streamon might give the wrong format.
> 
> Signed-off-by: Hans Verkuil 
> 
> diff --git a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> index bb656584..3695a027 100644
> --- a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> +++ b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
> @@ -2363,12 +2363,11 @@ static void stateful_m2m(cv4l_fd &fd,
> cv4l_queue &in, cv4l_queue &out,
> if (in_source_change_event) {
> in_source_change_event = false;
> last_buffer = false;
> +   fd.g_fmt(fmt_in, in.g_type());
> if (capture_setup(fd, in, exp_fd_p))
> return;
> fps_ts[CAP].reset();
> fps_ts[OUT].reset();
> -   fd.g_fmt(fmt_out, out.g_type());
> -   fd.g_fmt(fmt_in, in.g_type());
> Removing those lines cause inconsistency when the user send the wanted
> capture pixel format when decoding with the `v4l2-ctl` command. In
> this case the value of `-v pixelformat=...` is updated in the kernel
> with the capture_setup function but it is not updated in the fmt_in
> variable and so the command will try to save the decoded video in a
> different format from what is configured in the kernel.
> 
> cap_streaming = true;
> } else {
> break;
> 
> 
> Dafna
> 

It looks like you are right. Can you try this v4l2-ctl patch?

Regards,

Hans

[PATCH] v4l2-ctl: let capture_setup return the updated format

Just getting the new format after a source change event isn't
enough in the case that -v option was specified by the user.

Instead let capture_setup optionally return the new format after
any '-v' changes are applied.

Signed-off-by: Hans Verkuil 
Reported-by: Dafna Hirschfeld 
---
diff --git a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp 
b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
index 28b2b3b9..470fa766 100644
--- a/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl-streaming.cpp
@@ -2146,7 +2146,7 @@ enum stream_type {
OUT,
 };

-static int capture_setup(cv4l_fd &fd, cv4l_queue &in, cv4l_fd *exp_fd)
+static int capture_setup(cv4l_fd &fd, cv4l_queue &in, cv4l_fd *exp_fd, 
cv4l_fmt *new_fmt = NULL)
 {
if (fd.streamoff(in.g_type())) {
fprintf(stderr, "%s: fd.streamoff error\n", __func__);
@@ -2168,6 +2168,10 @@ static int capture_setup(cv4l_fd &fd, cv4l_queue &in, 
cv4l_fd *exp_fd)
return -1;
}
fd.s_fmt(fmt, in.g_type());
+   if (new_fmt)
+   *new_fmt = fmt;
+   } else if (new_fmt) {
+   fd.g_fmt(*new_fmt, in.g_type());
}
get_cap_compose_rect(fd);

@@ -2367,8 +2371,7 @@ static void stateful_m2m(cv4l_fd &fd, cv4l_queue &in, 
cv4l_queue &out,
if (in_source_change_event) {
in_source_change_event = false;
last_buffer = false;
-   fd.g_fmt(fmt_in, in.g_type());
-   if (capture_setup(fd, in, exp_fd_p))
+   if (capture_setup(fd, in, exp_fd_p, &fmt_in))
return;
fps_ts[CAP].reset();
fps_ts[OUT].reset();