After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 725632 - v4l2: Normalise control names in the same way as v4l2-ctl
v4l2: Normalise control names in the same way as v4l2-ctl
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.2.3
Other Linux
: Normal normal
: 1.3.1
Assigned To: Nicolas Dufresne (ndufresne)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-04 01:20 UTC by Will Manley
Modified: 2014-07-10 17:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to fix this issue (2.48 KB, patch)
2014-03-04 01:20 UTC, Will Manley
accepted-commit_now Details | Review
Misc typo fix (1.08 KB, patch)
2014-03-04 01:21 UTC, Will Manley
committed Details | Review
Patch including backwards compatibility (4.30 KB, patch)
2014-03-07 17:56 UTC, Will Manley
committed Details | Review
Patch including backwards compatibility (5.49 KB, patch)
2014-03-09 22:50 UTC, Will Manley
none Details | Review
Patch including backwards compatibility (5.06 KB, patch)
2014-03-09 23:14 UTC, Will Manley
rejected Details | Review
Patch including backwards compatibility for 1.2 stable series (4.26 KB, patch)
2014-03-09 23:21 UTC, Will Manley
none Details | Review
Patch against master making backwards compatibilty less liberal (5.46 KB, patch)
2014-03-10 16:53 UTC, Will Manley
needs-work Details | Review
Patch against master making backwards compatibilty less liberal (5.49 KB, patch)
2014-04-01 20:29 UTC, Will Manley
none Details | Review

Description Will Manley 2014-03-04 01:20:52 UTC
Created attachment 270862 [details] [review]
Patch to fix this issue

V4L2 kernel drivers allow configuration of the hardware settings via a
mechanism called controls.  These can be referred to by name such as
"Brightness" and "White Balance Temperature".  The user-space command line
client for setting these controls (v4l2-ctl) normalises these names such
that they only contain lower case alphanumeric characters and the
underscore '_'.  e.g:

    Kernel                     v4l2-ctl
    ----------------------------------------------------
    Brightness                 brightness
    White Balance Temperature  white_balance_temperature
    Focus (absolute)           focus_absolute

GStreamer seems to want to follow this pattern but failed for controls with
more than one consecutive non-alphanum character.  e.g. GStreamer would
produce "focus__absolute_" rather than "focus_absolute".

This commit fixes that issue.
Comment 1 Will Manley 2014-03-04 01:21:42 UTC
Created attachment 270863 [details] [review]
Misc typo fix
Comment 2 Nicolas Dufresne (ndufresne) 2014-03-04 02:47:40 UTC
Review of attachment 270862 [details] [review]:

I'm all in favour of doing that, small correction and we are good.

::: sys/v4l2/v4l2_calls.c
@@ +328,1 @@
         control.name[31] = '\0';

You can remove this one now, that was clearly a buf in previous code.
Comment 3 Nicolas Dufresne (ndufresne) 2014-03-04 02:48:57 UTC
Review of attachment 270863 [details] [review]:

All good, I'll merge this one tomorrow, at the same time with the other, thanks.
Comment 4 Will Manley 2014-03-04 13:47:37 UTC
(In reply to comment #2)
> Review of attachment 270862 [details] [review]:
> 
> I'm all in favour of doing that, small correction and we are good.
> 
> ::: sys/v4l2/v4l2_calls.c
> @@ +328,1 @@
>          control.name[31] = '\0';
> 
> You can remove this one now, that was clearly a buf in previous code.

I'm not sure what you mean by "a buf" but I think it's safer to keep this line
in.  The documentation to [VIDIOC_QUERYCTRL] says that name will be a NUL
terminated string so you're definitely not destroying any information here.  
OTOH this line does make us more robust to badly behaved kernel drivers -
avoiding the for loop from accidentally overrunning it's buffers if the string
is not NUL terminated.

This is purely theoretical of course.  I haven't seen any v4l2 drivers which
behave incorrectly in this way it just seemed safer to keep it.  I can remove it
if you like.

[VIDIOC_QUERYCTRL]: http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-queryctrl.html
Comment 5 Nicolas Dufresne (ndufresne) 2014-03-04 14:03:58 UTC
This was a typo, I mean a *bug*. The previous bug was that some uninitialized data could be seen between the last character and the 0 character.

You don't need it in your patch because you do this now, outside of your loop:
+        control.name[i++] = '\0';

Also, i cannot be bigger then 31, as the algo can only shorten the string, though you could add some safety here in case the driver don't set the 0 character.
Comment 6 Nicolas Dufresne (ndufresne) 2014-03-04 17:24:56 UTC
Comment on attachment 270862 [details] [review]
Patch to fix this issue

Sorry, got you on that, instead of checking the boundary in the loop we simply ensure there is a zero at the end.
Comment 7 Nicolas Dufresne (ndufresne) 2014-03-05 01:53:59 UTC
Ok, I've stepped back a bit, will poke upstream again, but this may be considered an ABI change, even though I think it's more correct.
Comment 8 Sebastian Dröge (slomo) 2014-03-05 19:18:12 UTC
It is an ABI change, yes. But seems more correct and less confusing. Maybe we can allow both for 1.4 (and print warnings for the old) and drop the old in 1.6?
Comment 9 Nicolas Dufresne (ndufresne) 2014-03-05 19:42:59 UTC
We would need to figure that out, maybe that could be done by running the normalizer on the gststructure names or something ?
Comment 10 Will Manley 2014-03-07 17:56:37 UTC
Created attachment 271257 [details] [review]
Patch including backwards compatibility

Uploading new patch.  Allows setting "exposure__auto" but emits the warning:

WARNING **: In GStreamer 1.4 the way V4L2 control names were normalised changed.  Instead of setting "exposure__auto" please use "exposure_auto".  The former is deprecated and will be removed in a future version of GStreamer

I'm not 100% happy with the patch as you could even use the key "Exposure, Auto" and it would still work.  e.g. it is *much* more liberal in what it accepts which may make it difficult to remove in the future.

What do you think?
Comment 11 Sebastian Dröge (slomo) 2014-03-08 14:25:57 UTC
I'd prefer if we would not allow even more liberal versions of the strings :) But a warning seems ok, if someone ignores these warnings then that's not our problem.
Comment 12 Nicolas Dufresne (ndufresne) 2014-03-09 00:18:48 UTC
(In reply to comment #11)
> I'd prefer if we would not allow even more liberal versions of the strings :)
> But a warning seems ok, if someone ignores these warnings then that's not our
> problem.

Good point, but so much simpler this way, the warning should do.
Comment 13 Nicolas Dufresne (ndufresne) 2014-03-09 00:40:57 UTC
Thanks

commit 4f47442c7f467a970e139871560af0ed4cb33ea0
Author: William Manley <will@williammanley.net>
Date:   Sat Mar 8 19:29:58 2014 -0500

    v4l2: Fix typo contol -> control
    
    https://bugzilla.gnome.org/show_bug.cgi?id=725632

commit d3bd3ecc3ebadf5f4871a34b9c268b1ac791cf06
Author: William Manley <will@williammanley.net>
Date:   Tue Mar 4 01:15:49 2014 +0000

    v4l2: Normalise control names in the same way as v4l2-ctl
    
    V4L2 kernel drivers allow configuration of the hardware settings via a
    mechanism called controls.  These can be referred to by name such as
    "Brightness" and "White Balance Temperature".  The user-space command line
    client for setting these controls (v4l2-ctl) normalises these names such
    that they only contain lower case alphanumeric characters and the
    underscore '_'.  e.g:
    
        Kernel                     v4l2-ctl
        ----------------------------------------------------
        Brightness                 brightness
        White Balance Temperature  white_balance_temperature
        Focus (absolute)           focus_absolute
    
    GStreamer seems to want to follow this pattern but failed for controls with
    more than one consecutive non-alphanum character.  e.g. GStreamer would
    produce "focus__absolute_" rather than "focus_absolute".
    
    This commit fixes that issue.  Backwards compatibility is preserved by
    normalising all control names before comparison.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=725632
Comment 14 Will Manley 2014-03-09 22:50:14 UTC
Created attachment 271391 [details] [review]
Patch including backwards compatibility
Comment 15 Will Manley 2014-03-09 23:14:37 UTC
Created attachment 271392 [details] [review]
Patch including backwards compatibility

(In reply to comment #11)
> I'd prefer if we would not allow even more liberal versions of the strings :)
> But a warning seems ok, if someone ignores these warnings then that's not our
> problem.

Agreed.  I've attached a patch which is more strict.  It's true that it's a little more complex, but I also think it's more self contained, documented and easier to remove in the future.

If this is preferred I'd recommend reverting d3bd3ec and applying this one instead.

I've also created a patch which could be applied to the 1.2 stable branch if acceptable.  It's just the same but there is no deprecation warning as it doesn't make sense to include that in a stable release.
Comment 16 Will Manley 2014-03-09 23:21:44 UTC
Created attachment 271393 [details] [review]
Patch including backwards compatibility for 1.2 stable series
Comment 17 Nicolas Dufresne (ndufresne) 2014-03-10 13:54:07 UTC
Comment on attachment 271393 [details] [review]
Patch including backwards compatibility for 1.2 stable series

I'll need the maintainers acknowledgement for this one. Will come back soon.
Comment 18 Will Manley 2014-03-10 15:39:07 UTC
Nicolas: I notice you've just marked attachment 271392 [details] [review] as committed but the version of the patch in gst-plugins-good[1] (as of now) is still d3bd3ec which corresponds to the older version of the patch in attachment 271257 [details] [review].

I would argue that 271392 is superior (see comment 15) but AFAICS it has not been pushed to master and would require first reverting d3bd3ec.  I thought I'd mention it in case we've got our wires crossed.

[1]: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/
Comment 19 Nicolas Dufresne (ndufresne) 2014-03-10 15:47:23 UTC
(In reply to comment #18)
> Nicolas: I notice you've just marked attachment 271392 [details] [review] as committed but the
> version of the patch in gst-plugins-good[1] (as of now) is still d3bd3ec which
> corresponds to the older version of the patch in attachment 271257 [details] [review].
> 
> I would argue that 271392 is superior (see comment 15) but AFAICS it has not
> been pushed to master and would require first reverting d3bd3ec.  I thought I'd
> mention it in case we've got our wires crossed.
> 
> [1]: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/

Sorry, I did not understood what you have tried to do, would it possible to submit a patch on top of what has been merged please ?
Comment 20 Nicolas Dufresne (ndufresne) 2014-03-10 15:50:13 UTC
Comment on attachment 271257 [details] [review]
Patch including backwards compatibility

We need to avoid marking obsolete patches marked commited. I personnally didn't care if it was slightly less restrictive then before, though it might be a concern for 1.2. I consider this as tempory code tha we'll remove, so doing more work on it is not that productive.
Comment 21 Nicolas Dufresne (ndufresne) 2014-03-10 15:51:26 UTC
Comment on attachment 271392 [details] [review]
Patch including backwards compatibility

Would need to be on top of what has been merged.
Comment 22 Will Manley 2014-03-10 16:53:44 UTC
Created attachment 271448 [details] [review]
Patch against master making backwards compatibilty less liberal

(In reply to comment #20)
> (From update of attachment 271257 [details] [review])
> We need to avoid marking obsolete patches marked commited. I personnally didn't
> care if it was slightly less restrictive then before, though it might be a
> concern for 1.2. I consider this as tempory code tha we'll remove, so doing
> more work on it is not that productive.

Sorry for the confusion.  I should have said earlier that I was working on another version of the patch so as to not waste your time.  I've attached a patch which can be applied against master to get the same effect as reverting d3bd3ec and applying the newer version of the patch.  As you say this is temporary and the reason I think the newer version of the patch is better is precisely because it is easier to remove in the future.

Commit message follows which I hope will make things clearer:

    v4l2: Make backwards compatibility code stricter
    
    d3bd3ec fixed an inconsistency in v4l2 control name normalisation
    between GStreamer and v4l2-ctl.  This included backwards compatibility
    code which means that applications which rely on the old normalisation
    scheme to continue working.  e.g. for the control "Exposure, Auto" both
    the new "exposure_auto" and the old "exposure__auto" will be accepted.
    
    Unfortunately the way this was implemented means that the code will now
    accept a *much* wider range of control names.  e.g. "exposure___auto",
    "__ExPoSuRe-_-AUto__" will both now be accepted (albiet with a warning).
    In the interests of making it easier to remove this backward
    compatibility hack in the future this patch narrows the acceptable names
    to just the old deprecated ones and the new ones.
    
    This patch also tidies the backwards compatibilty code up a bit making it
    more self-contained, easier to remove and better documented.  Backwards
    compatibility will disable itself in 1.5 (using preprocessor macros) and
    can then be removed.
    
    This is perhaps better considerred a fixup to d3bd3ec.
Comment 23 Nicolas Dufresne (ndufresne) 2014-03-27 19:26:07 UTC
Comment on attachment 271448 [details] [review]
Patch against master making backwards compatibilty less liberal

I've try applying, clearly it didn't work, then I reverted the old patch and try apply and did work. Would it be possible update this patch so it can be applied on git master, as a fixup of the previous one ?
Comment 24 Will Manley 2014-04-01 20:29:21 UTC
Created attachment 273430 [details] [review]
Patch against master making backwards compatibilty less liberal

(In reply to comment #23)
> (From update of attachment 271448 [details] [review])
> I've try applying, clearly it didn't work, then I reverted the old patch and
> try apply and did work. Would it be possible update this patch so it can be
> applied on git master, as a fixup of the previous one ?

Hmm, strange,

    $ curl https://bug725632.bugzilla-attachments.gnome.org/attachment.cgi?id=271448 \
          | git am -3

worked for me.  I guess we must have a different set of blobs in our git
repos defeating git's merge algorithms.

Anyway, I've created a new version of this patch rebased on master which
should hopefully work which is attached.
Comment 25 Nicolas Dufresne (ndufresne) 2014-06-04 13:38:03 UTC
Slomo, how much do you care about this compat thing ? Shall I merge, or simply keep it the way it is now (permissive) ?
Comment 26 Sebastian Dröge (slomo) 2014-06-04 13:40:16 UTC
I think it's fine either way, don't have a strong opinion
Comment 27 Nicolas Dufresne (ndufresne) 2014-07-10 17:01:13 UTC
Let's leave it this way. It's simple and inoffensive.