GNOME Bugzilla – Bug 725632
v4l2: Normalise control names in the same way as v4l2-ctl
Last modified: 2014-07-10 17:01:13 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.
Created attachment 270863 [details] [review] Misc typo fix
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.
Review of attachment 270863 [details] [review]: All good, I'll merge this one tomorrow, at the same time with the other, thanks.
(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
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 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.
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.
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?
We would need to figure that out, maybe that could be done by running the normalizer on the gststructure names or something ?
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?
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.
(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.
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
Created attachment 271391 [details] [review] Patch including backwards compatibility
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.
Created attachment 271393 [details] [review] Patch including backwards compatibility for 1.2 stable series
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.
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/
(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 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 on attachment 271392 [details] [review] Patch including backwards compatibility Would need to be on top of what has been merged.
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 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 ?
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.
Slomo, how much do you care about this compat thing ? Shall I merge, or simply keep it the way it is now (permissive) ?
I think it's fine either way, don't have a strong opinion
Let's leave it this way. It's simple and inoffensive.