On Wed, 18 Jun 2014 15:59:13 +0200 Hans de Goede <hdego...@redhat.com> wrote:
> Hi, > > On 06/18/2014 03:23 PM, Antonio Ospite wrote: > > On Wed, 18 Jun 2014 13:46:10 +0200 > > Hans de Goede <hdego...@redhat.com> 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? -- Antonio Ospite http://ao2.it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
