On 12/22/18 10:03 AM, Dafna Hirschfeld wrote: > On Wed, Dec 19, 2018 at 12:03 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >> >> Yes, but those values are used in ioctl calls to the driver, so rather >> than using those values you query the driver. >> >>> They are needed in order to >>> read raw frames line by line for the encoder. >> >> Why not call G_FMT and G_SELECTION(TGT_CROP) to obtain that information? >> > The way vicodec is now implemented is that it does not hold the > original width/height given in S_FMT but the coded once. > But the userspace still needs these values in order to read raw frames > from the file. > Not sure how to solve it. When running v4l2-ctl you have to set both the width and height with the -x option, AND you set the selection crop rectangle to that same width and height. So the selection crop rectangle has the actual visible width and height. In fact, for a video output device in general you can call G_SELECTION(TGT_CROP) and use the returned width and height when reading from the file, or (if not supported) you use the width and height returned by G_FMT. > Why does the userspace encoder need to query this values from the > driver ? Since these are raw frames the userspace > should already know the dimensions. This assumes that the user explicitly provides the visible width and height, but that doesn't have to be the case. In which case the drivers values should be used. Also, m2m devices are special in that the configuration is stored per-filehandle, but that is not case for regular capture and output devices: the configuration is per device in that case, so you can set the width and height with v4l2-ctl and then in a new v4l2-ctl command query the width and height and you'll get back the width and height you set previously. Just try this with e.g. vivid and vim2m and see the difference in behavior. BTW, right now when we set the output width and height for the encoder with the -x option we do not set the CROP rectangle with that width and height, we only clamp any existing rectangle. This is right according to the current V4L2 spec, but it is not clear if this behavior makes sense for codecs. In the new year this is something we should discuss with others. Perhaps S_FMT for an encoder should automatically set the CROP target for the output as well using the original S_FMT width and height. Regards, Hans > > Dafna > >> Please note that all the set and get options are all processed before the >> streaming options. So when you start streaming the driver is fully configured. >> >>> Maybe I can implement it by calling 'parse_fmt' in 'stream_cmd' >>> function similar to how 'vidout_cmd' do it. >>> >>> https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-ctl/v4l2-ctl-vidout.cpp#n90 >>> >>> >>>> Remember that you can call v4l2-ctl without setting the output width and height >>>> if the defaults that the driver sets are already fine. In that case the width and height >>>> variables in this source are just 0. >>>> >>>>> + >>>>> void vidout_list(cv4l_fd &fd) >>>>> { >>>>> if (options[OptListOutFormats]) { >>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl.h b/utils/v4l2-ctl/v4l2-ctl.h >>>>> index 5a52a0a4..ab2994b2 100644 >>>>> --- a/utils/v4l2-ctl/v4l2-ctl.h >>>>> +++ b/utils/v4l2-ctl/v4l2-ctl.h >>>>> @@ -357,6 +357,8 @@ void vidout_cmd(int ch, char *optarg); >>>>> void vidout_set(cv4l_fd &fd); >>>>> void vidout_get(cv4l_fd &fd); >>>>> void vidout_list(cv4l_fd &fd); >>>>> +void vidcap_get_orig_from_set(unsigned int &r_width, unsigned int &r_height); >>>>> +void vidout_get_orig_from_set(unsigned int &r_width, unsigned int &r_height); >>>>> >>>>> // v4l2-ctl-overlay.cpp >>>>> void overlay_usage(void); >>>>> >>>> >>>> This patch needs more work (not surprisingly, since it takes a bit of time to >>>> understand the v4l2-ctl source code). >>>> >>>> Please stick to the kernel coding style! Using a different style makes it harder >>>> for me to review since my pattern matches routines in my brain no longer work >>>> optimally. It's like reading text with spelling mistakes, you cn stil undrstant iT, >>>> but it tekes moore teem. :-) >>>> >>> okei :) >>> >>>> Regards, >>>> >>>> Hans >> >> Regards, >> >> Hans
