* [PATCH] libv4lconvert: Fix a regression when converting from Y10B @ 2014-06-03 13:48 Antonio Ospite 2014-06-03 13:59 ` Antonio Ospite 0 siblings, 1 reply; 9+ messages in thread From: Antonio Ospite @ 2014-06-03 13:48 UTC (permalink / raw) To: linux-media; +Cc: Antonio Ospite, Gregor Jasny Fix a regression introduced in commit efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too short source buffer before accessing it). The old code: case V4L2_PIX_FMT_Y10BPACK: ... if (result == 0 && src_size < (width * height * 10 / 8)) { V4LCONVERT_ERR("short y10b data frame\n"); errno = EPIPE; result = -1; } ... meant to say "If the conversion was *successful* _but_ the frame size was invalid, then take the error path", but in efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was misunderstood and the v4lconvert_convert_pixfmt() was made to return an error even in the case of a successful conversion from Y10B. Fix the check, and now print only the message letting the errno and the result from the conversion routines to be propagated to the caller. Signed-off-by: Antonio Ospite <ao2@ao2.it> Cc: Gregor Jasny <gjasny@googlemail.com> --- Hi, the regression affects only the users of the gspca-kinect driver when using the IR stream. I think this should be cherry-picked in stable-1.0 too. BTW Gregor, in efc29f1764a30808ebf7b3e1d9bfa27b909bf641 you say "Reject too short source buffer before accessing it" but the code only does "result = -1" and then still calls the conversion routines, so it's not actually *rejecting* the frames, am I missing anything? Thanks, Antonio lib/libv4lconvert/libv4lconvert.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c index c49d30d..50d6906 100644 --- a/lib/libv4lconvert/libv4lconvert.c +++ b/lib/libv4lconvert/libv4lconvert.c @@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, width, height); break; } - if (result == 0) { + if (result != 0) V4LCONVERT_ERR("y10b conversion failed\n"); - errno = EPIPE; - result = -1; - } break; case V4L2_PIX_FMT_RGB565: -- 2.0.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] libv4lconvert: Fix a regression when converting from Y10B 2014-06-03 13:48 [PATCH] libv4lconvert: Fix a regression when converting from Y10B Antonio Ospite @ 2014-06-03 13:59 ` Antonio Ospite 2014-06-16 15:00 ` [PATCH RESEND] " Antonio Ospite 0 siblings, 1 reply; 9+ messages in thread From: Antonio Ospite @ 2014-06-03 13:59 UTC (permalink / raw) To: linux-media; +Cc: Antonio Ospite, Gregor Jasny On Tue, 3 Jun 2014 15:48:46 +0200 Antonio Ospite <ao2@ao2.it> wrote: > Fix a regression introduced in commit > efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too > short source buffer before accessing it). > > The old code: > > case V4L2_PIX_FMT_Y10BPACK: > ... > if (result == 0 && src_size < (width * height * 10 / 8)) { > V4LCONVERT_ERR("short y10b data frame\n"); > errno = EPIPE; > result = -1; > } > ... > > meant to say "If the conversion was *successful* _but_ the frame size > was invalid, then take the error path", but in > efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was > misunderstood and the v4lconvert_convert_pixfmt() was made to return an ^^^ Dear committer, you can remove this "the", if you feel like it :) Thanks, Antonio -- 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-03 13:59 ` Antonio Ospite @ 2014-06-16 15:00 ` Antonio Ospite 2014-06-18 11:43 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Antonio Ospite @ 2014-06-16 15:00 UTC (permalink / raw) To: linux-media; +Cc: Antonio Ospite, Hans de Goede, Gregor Jasny Fix a regression introduced in commit efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too short source buffer before accessing it). The old code: case V4L2_PIX_FMT_Y10BPACK: ... if (result == 0 && src_size < (width * height * 10 / 8)) { V4LCONVERT_ERR("short y10b data frame\n"); errno = EPIPE; result = -1; } ... meant to say "If the conversion was *successful* _but_ the frame size was invalid, then take the error path", but in efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was misunderstood and v4lconvert_convert_pixfmt() was made to return an error even in the case of a successful conversion from Y10B. Fix the check, and now print only the message letting the errno and the result from the conversion routines to be propagated to the caller. Signed-off-by: Antonio Ospite <ao2@ao2.it> Cc: Gregor Jasny <gjasny@googlemail.com> --- lib/libv4lconvert/libv4lconvert.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c index c49d30d..50d6906 100644 --- a/lib/libv4lconvert/libv4lconvert.c +++ b/lib/libv4lconvert/libv4lconvert.c @@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, width, height); break; } - if (result == 0) { + if (result != 0) V4LCONVERT_ERR("y10b conversion failed\n"); - errno = EPIPE; - result = -1; - } break; case V4L2_PIX_FMT_RGB565: -- 2.0.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-16 15:00 ` [PATCH RESEND] " Antonio Ospite @ 2014-06-18 11:43 ` Hans de Goede 2014-06-18 11:46 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2014-06-18 11:43 UTC (permalink / raw) To: Antonio Ospite, linux-media; +Cc: Gregor Jasny Hi, On 06/16/2014 05:00 PM, Antonio Ospite wrote: > Fix a regression introduced in commit > efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too > short source buffer before accessing it). > > The old code: > > case V4L2_PIX_FMT_Y10BPACK: > ... > if (result == 0 && src_size < (width * height * 10 / 8)) { > V4LCONVERT_ERR("short y10b data frame\n"); > errno = EPIPE; > result = -1; > } > ... > > meant to say "If the conversion was *successful* _but_ the frame size > was invalid, then take the error path", but in > efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was > misunderstood and v4lconvert_convert_pixfmt() was made to return an > error even in the case of a successful conversion from Y10B. > > Fix the check, and now print only the message letting the errno and the > result from the conversion routines to be propagated to the caller. > > Signed-off-by: Antonio Ospite <ao2@ao2.it> > Cc: Gregor Jasny <gjasny@googlemail.com> Thanks for the patch, but: ... > --- > lib/libv4lconvert/libv4lconvert.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c > index c49d30d..50d6906 100644 > --- a/lib/libv4lconvert/libv4lconvert.c > +++ b/lib/libv4lconvert/libv4lconvert.c > @@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, > width, height); > break; > } > - if (result == 0) { > + if (result != 0) > V4LCONVERT_ERR("y10b conversion failed\n"); > - errno = EPIPE; > - result = -1; > - } > break; > > case V4L2_PIX_FMT_RGB565: 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? 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. Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-18 11:43 ` Hans de Goede @ 2014-06-18 11:46 ` Hans de Goede 2014-06-18 13:23 ` Antonio Ospite 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2014-06-18 11:46 UTC (permalink / raw) To: Antonio Ospite, linux-media; +Cc: Gregor Jasny Hi, On 06/18/2014 01:43 PM, Hans de Goede wrote: > Hi, > > On 06/16/2014 05:00 PM, Antonio Ospite wrote: >> Fix a regression introduced in commit >> efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too >> short source buffer before accessing it). >> >> The old code: >> >> case V4L2_PIX_FMT_Y10BPACK: >> ... >> if (result == 0 && src_size < (width * height * 10 / 8)) { >> V4LCONVERT_ERR("short y10b data frame\n"); >> errno = EPIPE; >> result = -1; >> } >> ... >> >> meant to say "If the conversion was *successful* _but_ the frame size >> was invalid, then take the error path", but in >> efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was >> misunderstood and v4lconvert_convert_pixfmt() was made to return an >> error even in the case of a successful conversion from Y10B. >> >> Fix the check, and now print only the message letting the errno and the >> result from the conversion routines to be propagated to the caller. >> >> Signed-off-by: Antonio Ospite <ao2@ao2.it> >> Cc: Gregor Jasny <gjasny@googlemail.com> > > Thanks for the patch, but: ... > >> --- >> lib/libv4lconvert/libv4lconvert.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c >> index c49d30d..50d6906 100644 >> --- a/lib/libv4lconvert/libv4lconvert.c >> +++ b/lib/libv4lconvert/libv4lconvert.c >> @@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, >> width, height); >> break; >> } >> - if (result == 0) { >> + if (result != 0) >> V4LCONVERT_ERR("y10b conversion failed\n"); >> - errno = EPIPE; >> - result = -1; >> - } >> break; >> >> case V4L2_PIX_FMT_RGB565: > > 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? > > 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. Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-18 11:46 ` Hans de Goede @ 2014-06-18 13:23 ` Antonio Ospite 2014-06-18 13:59 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Antonio Ospite @ 2014-06-18 13:23 UTC (permalink / raw) To: Hans de Goede; +Cc: linux-media, Gregor Jasny On Wed, 18 Jun 2014 13:46:10 +0200 Hans de Goede <hdegoede@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: > >> Fix a regression introduced in commit > >> efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too > >> short source buffer before accessing it). > >> > >> The old code: > >> > >> case V4L2_PIX_FMT_Y10BPACK: > >> ... > >> if (result == 0 && src_size < (width * height * 10 / 8)) { > >> V4LCONVERT_ERR("short y10b data frame\n"); > >> errno = EPIPE; > >> result = -1; > >> } > >> ... > >> > >> meant to say "If the conversion was *successful* _but_ the frame size > >> was invalid, then take the error path", but in > >> efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was > >> misunderstood and v4lconvert_convert_pixfmt() was made to return an > >> error even in the case of a successful conversion from Y10B. > >> > >> Fix the check, and now print only the message letting the errno and the > >> result from the conversion routines to be propagated to the caller. > >> > >> Signed-off-by: Antonio Ospite <ao2@ao2.it> > >> Cc: Gregor Jasny <gjasny@googlemail.com> > > > > Thanks for the patch, but: ... > > > >> --- > >> lib/libv4lconvert/libv4lconvert.c | 5 +---- > >> 1 file changed, 1 insertion(+), 4 deletions(-) > >> > >> diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c > >> index c49d30d..50d6906 100644 > >> --- a/lib/libv4lconvert/libv4lconvert.c > >> +++ b/lib/libv4lconvert/libv4lconvert.c > >> @@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, > >> width, height); > >> break; > >> } > >> - if (result == 0) { > >> + if (result != 0) > >> V4LCONVERT_ERR("y10b conversion failed\n"); > >> - errno = EPIPE; > >> - result = -1; > >> - } > >> break; > >> > >> case V4L2_PIX_FMT_RGB565: > > > > 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. 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? Ciao ciao, Antonio -- 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-18 13:23 ` Antonio Ospite @ 2014-06-18 13:59 ` Hans de Goede 2014-06-18 14:40 ` Antonio Ospite 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2014-06-18 13:59 UTC (permalink / raw) To: Antonio Ospite; +Cc: linux-media, Gregor Jasny Hi, On 06/18/2014 03:23 PM, Antonio Ospite wrote: > On Wed, 18 Jun 2014 13:46:10 +0200 > Hans de Goede <hdegoede@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: >>>> Fix a regression introduced in commit >>>> efc29f1764a30808ebf7b3e1d9bfa27b909bf641 (libv4lconvert: Reject too >>>> short source buffer before accessing it). >>>> >>>> The old code: >>>> >>>> case V4L2_PIX_FMT_Y10BPACK: >>>> ... >>>> if (result == 0 && src_size < (width * height * 10 / 8)) { >>>> V4LCONVERT_ERR("short y10b data frame\n"); >>>> errno = EPIPE; >>>> result = -1; >>>> } >>>> ... >>>> >>>> meant to say "If the conversion was *successful* _but_ the frame size >>>> was invalid, then take the error path", but in >>>> efc29f1764a30808ebf7b3e1d9bfa27b909bf641 this (maybe weird) logic was >>>> misunderstood and v4lconvert_convert_pixfmt() was made to return an >>>> error even in the case of a successful conversion from Y10B. >>>> >>>> Fix the check, and now print only the message letting the errno and the >>>> result from the conversion routines to be propagated to the caller. >>>> >>>> Signed-off-by: Antonio Ospite <ao2@ao2.it> >>>> Cc: Gregor Jasny <gjasny@googlemail.com> >>> >>> Thanks for the patch, but: ... >>> >>>> --- >>>> lib/libv4lconvert/libv4lconvert.c | 5 +---- >>>> 1 file changed, 1 insertion(+), 4 deletions(-) >>>> >>>> diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c >>>> index c49d30d..50d6906 100644 >>>> --- a/lib/libv4lconvert/libv4lconvert.c >>>> +++ b/lib/libv4lconvert/libv4lconvert.c >>>> @@ -1052,11 +1052,8 @@ static int v4lconvert_convert_pixfmt(struct v4lconvert_data *data, >>>> width, height); >>>> break; >>>> } >>>> - if (result == 0) { >>>> + if (result != 0) >>>> V4LCONVERT_ERR("y10b conversion failed\n"); >>>> - errno = EPIPE; >>>> - result = -1; >>>> - } >>>> break; >>>> >>>> case V4L2_PIX_FMT_RGB565: >>> >>> 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. > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-18 13:59 ` Hans de Goede @ 2014-06-18 14:40 ` Antonio Ospite 2014-06-19 8:27 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Antonio Ospite @ 2014-06-18 14:40 UTC (permalink / raw) To: Hans de Goede; +Cc: linux-media, Gregor Jasny On Wed, 18 Jun 2014 15:59:13 +0200 Hans de Goede <hdegoede@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 <hdegoede@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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] libv4lconvert: Fix a regression when converting from Y10B 2014-06-18 14:40 ` Antonio Ospite @ 2014-06-19 8:27 ` Hans de Goede 0 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2014-06-19 8:27 UTC (permalink / raw) To: Antonio Ospite; +Cc: linux-media, Gregor Jasny Hi, On 06/18/2014 04:40 PM, Antonio Ospite wrote: > On Wed, 18 Jun 2014 15:59:13 +0200 > Hans de Goede <hdegoede@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 <hdegoede@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? cherry-picked and pushed. Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-06-19 8:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-03 13:48 [PATCH] libv4lconvert: Fix a regression when converting from Y10B Antonio Ospite 2014-06-03 13:59 ` Antonio Ospite 2014-06-16 15:00 ` [PATCH RESEND] " Antonio Ospite 2014-06-18 11:43 ` Hans de Goede 2014-06-18 11:46 ` Hans de Goede 2014-06-18 13:23 ` Antonio Ospite 2014-06-18 13:59 ` Hans de Goede 2014-06-18 14:40 ` Antonio Ospite 2014-06-19 8:27 ` Hans de Goede
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.