drivers: video: common: fix video_closest_frmival() fie.index #89674
Conversation
Fix bug introduced in 46a262f where the fie.index field was expected to be incremented by the driver, while it is the responsibility of the caller to increment it. Fixes zephyrproject-rtos#89663 Signed-off-by: Josuah Demangeon <me@josuah.net>
josuah
added
area: Drivers
area: Tests
|
Example of implementation that correctly keep Lines 1195 to 1221 in af77efb |
|
As suggested, a second commit for a small optimization for exact matches. |
drivers/video/video_common.c
Outdated
| @@ -153,10 +153,16 @@ void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, | |||
| a = video_frmival_nsec(&desired); | |||
There was a problem hiding this comment.
One more small optimization if I get it right. This first calculation of the desired frmival (a) only has to be done once, so should be done before entering the while loop.
drivers/video/video_common.c
Outdated
| @@ -153,10 +153,16 @@ void video_closest_frmival(const struct device *dev, enum video_endpoint_id ep, | |||
| a = video_frmival_nsec(&desired); | |||
| b = video_frmival_nsec(&tmp); | |||
| diff_nsec = a > b ? a - b : b - a; | |||
|
|
|||
| if (diff_nsec < best_diff_nsec) { | |||
| best_diff_nsec = diff_nsec; | |||
| match->index = fie.index; | |||
| memcpy(&match->discrete, &tmp, sizeof(tmp)); | |||
There was a problem hiding this comment.
what about simply doing match->discrete = tmp here ? Meaning, simply letting the compiler decide what is the best way to do that, and figure out optimization, depending on alignments etc. There is good chances that it would end up to be same as memcpy in this case but this is also I think more easy to read.
There was a problem hiding this comment.
Much better I agree!
josuah
force-pushed
the
pr-video-fix-frmival
branch
from
6eb2646 to
c32f8ff
Compare
|
force-push:
|
josuah
force-pushed
the
pr-video-fix-frmival
branch
from
c32f8ff to
bd176c2
Compare
In video_closest_frmival(), immediately stop searching when an exact match is found, as a small performance optimization. Variables that could be computed only once were moved further outside. Signed-off-by: Josuah Demangeon <me@josuah.net>
The assert message was not very helpful, CODE_UNREACHABLE is more readable and requires fewer effort while proofreading. Signed-off-by: Josuah Demangeon <me@josuah.net>
josuah
force-pushed
the
pr-video-fix-frmival
branch
from
bd176c2 to
df243bc
Compare
|
Downstream:
Fix bug introduced in 46a262f where the fie.index field was expected to be incremented by the driver, while it is the responsibility of the caller to increment it.
Fixes #89663
Split out of #88003