Hi, On 06/18/2014 04:40 PM, Antonio Ospite wrote: > On Wed, 18 Jun 2014 15:59:13 +0200 > Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > >> Hi, >> >> On 06/18/2014 03:23 PM, Antonio Ospite wrote: >>> On Wed, 18 Jun 2014 13:46:10 +0200 >>> Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>> >>>> Hi, >>>> >>>> On 06/18/2014 01:43 PM, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 06/16/2014 05:00 PM, Antonio Ospite wrote: > [...] >>>>> Why print a message here at all in the != 0 case? In the old code before commit >>>>> efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, so >>>>> I assume that that already does a V4LCONVERT_ERR in that case. So why do it a >>>>> second time with a less precise error message here? >>>>> >>> >>> The one from v4lconvert_oom_error(), yes, which is generic, it does not >>> tell _where_ the failure was. >>> >>>>> So I believe that the proper fix would be to just remove the entire block instead >>>>> of flipping the test and keeping the V4LCONVERT_ERR. Please send a new version >>>>> with this fixed, then I'll merge it asap. >>>> >>>> Scrap that, I decided I might just as well fix this bit myself, so I've just >>>> pushed an updated patch completely removing the second check from the >>>> V4L2_PIX_FMT_Y10BPACK case. >>>> >>> >>> The rationale behind leaving the message was: >>> 1. The conversion routines are called even in the case of short >>> frames (BTW that is true for any format, not just for Y10B). >>> 2. The conversion routines from Y10B are not "in place", they >>> allocate a temporary buffer, so they may fail themselves. >> >> Right, and this already does a V4LCONVERT_ERR, which will override any >> error msg stored earlier. >> > > I see now, I was overlooking how V4LCONVERT_ERR() works. > >>> with this in mind I saw the second message as an _additional_ error >>> indication to the user (useful in case of short frame _and_ conversion >>> failure) rather than a less precise one. However, you are right that >>> this additional error message was not in the original code before >>> efc29f1764, so your patch is perfectly fine by me. >>> >>> Thanks for merging it. >>> >>> BTW, comments about 1.? >>> What's the idea behind calling the conversion routines even for short >>> frames? >> >> For short frames the higher layer (libv4l2) will retry up to 3 times and then >> just return whatever it did get. The src_size is the amount of available bytes >> in the source buffer, the actual source buffer is pre-allocated and is always >> large enough, so in case of 3 consecutive short frames we convert whatever we >> did get + whatever data there was already in the buffer for the rest of the >> frame and return that to the user. >> >> This is useful since if the vsync timing is off between bridge and sensor, >> we often miss some lines at the bottom. So by converting what ever we do get we >> end up returning a frame with a mostly complete picture + 2 lines of garbage at >> the bottom at 1/3th of the framerate because of the retries. >> >> Ideally this would never happen, but it does and in this case actually showing >> the broken picture and allowing the user to take a screenshot of this and >> attach it to a bug report makes things a whole lot easier to debug. And in this >> case the camera is even still somewhat usable by the user this way. >> >> Likewise in other cases where the driver consistently feeds us short frames, >> it can be quite helpful to actually see the contents of the short frame >> for debugging purposes. >> >> Regards, >> >> Hans >> > > Thanks for the explanation. > > Ciao, > Antonio > > P.S. can we please have commit fff7e0eaae9734aa1f0a4e0fadef0d8c5c41b1e8 > cherry-picked in the stable-1.0 branch? cherry-picked and pushed. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
