Re: Question about drivers/media/usb/uvc/uvc_v4l2.c

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Shoabo,

On Sat, Mar 02, 2019 at 03:29:25PM -0700, Shaobo He wrote:
> Hi Laurent,
> 
> Thank you very much for your reply. This is what I thought, too. It seems that 
> there's an implicit order of execution that is not clearly implied in the code, 
> meaning `uvc_parse_streaming` is called before `uvc_v4l2_try_format`.

It is implied by the logic of the driver that calls
uvc_parse_streaming() at probe time before registering video nodes, and
with uvc_v4l2_try_format() only being called from userspace through the
video nodes.

> That being said, I was wondering maybe a better practice to write the loop in 
> `uvc_v4l2_try_format` would be like the following,
> 
> ```
> format=NULL;
> ...
> for (i = 0; i < stream->nformats; ++i) {
> 		format = &stream->format[i];
> 		if (format->fcc == fmt->fmt.pix.pixelformat)
> 			break;
> }
> // dereferencing format
> ```
> to
> ```
> // just declaration
> format;
> i=0;
> do {
> 		format = &stream->format[i];
> 		if (format->fcc == fmt->fmt.pix.pixelformat)
> 			break;
> 		++i;
> } while (i<stream->nformats)
> // dereferencing format
> ```
> I mean you can save one initialization, provided compiler does it and one branch.

I like for loops better in general, they convey the meaning in a cleaner
way. The compiler should be able to do its job here and optimize the
code correctly, I don't think a change is worth it, especially as we're
not dealing with a hot path.

> On 2019/3/2 14:43, Laurent Pinchart wrote:
> > On Sat, Mar 02, 2019 at 01:22:49PM -0700, Shaobo He wrote:
> >> Hello everyone,
> >>
> >> This is Shaobo from Utah again. I've been bugging the mailing list with my
> >> patches. I have a quick question about a function in
> >> `drivers/media/usb/uvc/uvc_v4l2.c`. In `uvc_v4l2_try_format`, can
> >> `stream->nformats` be 0? I saw that in other files, this field could be zero
> >> which is considered as error cases. I was wondering if it's true for this
> >> function, too.
> > 
> > The uvc_parse_streaming() function should answer this question :-)

-- 
Regards,

Laurent Pinchart