Problem in `VIDIOC_G/TRY/S_FMT` on `allow_scaling=1` and `allow_cropping=1`

[kevinshieh0225]

There is a few problems in VIDIOC_G/TRY/S_FMT implementation. Some issue result in test fail on v4l2-compliance, others comes from that the implement is incompliant with document description, and nevertheless some questions from my own.

allow_scaling=1

1. allow_scaling=1 will lead to test failure on VIDIOC_G/TRY/S_FMT (710). The issue comes from the inconsistency, which the return format first acquire from G_FMT should match the later returning format from TRY_FMT.

   createInvalidFmt(fmt, clip, type);
   doioctl(node, VIDIOC_G_FMT, &fmt);
   fmt_try = fmt;
   ret = doioctl(node, VIDIOC_TRY_FMT, &fmt_try);
   ...
   if (!matchFormats(fmt, fmt_try))
   result = fail("%s: TRY_FMT(G_FMT) != G_FMT\n",
           buftype2s(type).c_str());

2. While vcam device with parameter setting allow_scaling=1, TRY_FMT would force to change the ouputbuffer and vcamfb size to {1280, 720}, I am curious that is it intended?

   /* find the nearest size from {vcam_sizes[n_avail - 1].width, vcam_sizes[n_avail - 1].height}
   * which is specify to {1280, 720}
   */
   const struct v4l2_frmsize_discrete *sz = v4l2_find_nearest_size(
           vcam_sizes, n_avail, width, height, vcam_sizes[n_avail - 1].width,
           vcam_sizes[n_avail - 1].height);

3. If the behavior is intended, there will be two problem for the implementation:
   • 3.1 According to the document, TRY_FMT should not change the driver state. The job should transfer to S_FMT to accomplish.

     The VIDIOC_TRY_FMT ioctl is equivalent to VIDIOC_S_FMT with one exception: it does not change driver state. It can also be called at any time, never returning EBUSY. This function is provided to negotiate parameters, to learn about hardware limitations, without disabling I/O or possibly time consuming hardware preparations.

• 3.2 When allow_scaling=1, TRY_FMT will always return fmt with size={1280, 720}. Than test [710] will always fail if default size was not {1280, 720}.

allow_cropping=1

4. dev->crop_output_format is first assign in TRY_FMT. However in practice, application would first call G_FMT to query current format, than further call S_FMT to modify the desire format. The problem lead to test fail in (441) and also (710). The assignment should be happen before G_FMT were called. (In my opinion, it might be reasonable to assign while create_vcam_device).

   Good practice is to query the current parameters first, and to modify only those parameters not suitable for the application.

vcamfb_update

5. This is a question from me only. I am not sure why it is necessary to update the vcamfb after TRY/S_FMT, while the device can handle different input and output buffer format in submit_copy_buffer
6. Finally, what is the difference between fb.c/vcamfb_update and control.c/control_iocontrol_modify_input_setting, in my opinion, the two function has similar purpose and might be a chance to combine together?

[WayneLin1992]

2. you can set

   const struct v4l2_frmsize_discrete *sz = v4l2_find_nearest_size(
           vcam_sizes, n_avail, width, height, f->fmt.pix.width,
           f->fmt.pix.height);

   , and you would pass the scaling test on v4l2-compliance.

   3. TRY_FMT should not change the driver state, but it could reset the size. reference vivid_try_fmt_vid_cap The vivid is a Linux kernel test device driver and pass all v4l2-compliance test.
   4. fb.c/vcamfb_update can remalloc space for the sizeimage, and set var.xres var.yres actually seen on the window. The var.xres_virtual and var.yres_virtual are real space size in memory. reference The Frame Buffer Device 6.control.c/control_iocontrol_modify_input_setting use the old API need to change.

[kevinshieh0225]

Thanks for @WayneLin1992 's reply!

3. I think vivid_try_fmt_vid_cap only fill in negotiate information to user, but didn't actually reset the size setting from the device. vivid_s_fmt_vid_cap is the function which change the device setting. I still think it is better to change the behavior according to the API description.
5. Is it better to bind two update function vcamfb_update and vcam_in_queue_destroy/setup together? Which in former function rebuild vcamfb but only change in_queue attribute, and the latter one only rebuild in_queue without vcamfb.
6. This question extend from question 5: if we want to change the in/out_queue setting, is it necessary to destroy and init a new in/out_queue, or we only need to change the attribute only? And this is the reason why in your vcamfb_update implementation, you only change the attribute on in/out_queue instead of re-init them?

[WayneLin1992]

Thanks for @kevinshieh0225

3. Your ideal is correct. TRY_FMT it will fill format negotiated for device, and S_FMT is actually reset the device size from format.
5. Your ideal is correct. vcam_in_queue_destroy/setup is old API for vcamfb, so it need to combine with vcamfb_update.
6. You only need to change the attribute on in_queue.