There is also a possible issue if different formats supports different framerates.
A solution could be to look for the format which supports the highest framerate and save just that.

I just tested this with my camera (Logitech Quckcam Orbit AF using uvcvideo driver) and I don't see this. What driver are you using?

Thanks

I have both a v4l2 uvcvideo based webcam that supports a single yuv format (YUY2)
and a Logitech Quickcam Express Plus that is supported by gspca drivers and have this issue.
That's because gspca exposes several video_palettes for each camera (http://mxhaard.free.fr/sview.html#SECTION00023000000000000000).
>Palette setting we courrently support for all cameras :
>
>    * VIDEO_PALETTE_RGB565 16 Bits Video palette
>    * VIDEO_PALETTE_RGB24 24 Bits Video palette
>    * VIDEO_PALETTE_RGB32 32 Bits Video palette
>    * VIDEO_PALETTE_YUV420P Planar yuv 4.2.0 palette
>    * VIDEO_PALETTE_RAWJPEG 8 Bits raw stream


So at least all gspca based have this issue but other devices can probably have it too.

Created attachment 117315 [details] [review]
CheeseVideoFormat refactoring

This patch should solve this bug and the related one, please double check it since I wrote it quickly and I won't be able to work on it anymore before the 2nd week of September (taking some vacation).

A little summary of what I did:
- normalize caps so that each gststructure has a single framerate value (easier to find the maximum one available).
- save only unique videoformats (mimetype, width, height) with the maximum framerate (why save a list of framerates if we only use the maximum one?)
- if a unsupported format is set through gconf try to find the most similar one in the supported formats list
- if no format is set through gconf use best available format for the current webcam
- avoid duplicates in the preferences dialog using "VideoFormat" (mimetype@widthxheight) items in the resolution combo

I think we should have per webcam settings in gconf since each device supports different formats. This would also allow to skip format detection for "known webcams" at startup saving a lot of time.

(In reply to comment #4)
> Created an attachment (id=117315) [edit]
> CheeseVideoFormat refactoring
> 
> This patch should solve this bug and the related one, please double check it
> since I wrote it quickly and I won't be able to work on it anymore before the
> 2nd week of September (taking some vacation).
Lucky you...I get to start class tomorrow. ;)

I took a look at the patch, but some of the assumptions you make concern me: 
> A little summary of what I did:
> - normalize caps so that each gststructure has a single framerate value (easier
> to find the maximum one available).
> - save only unique videoformats (mimetype, width, height) with the maximum
> framerate (why save a list of framerates if we only use the maximum one?)
I don't think this is going to be the case forever, so assuming this is a bad idea. When I was designing the prefs system one of the things that Jaap wanted was the ability to select framerates. We also talked about the idea of selecting video formats too. Your solution would make implementing these very hard, and we'd probably have to come back and refactor it again 
> - if a unsupported format is set through gconf try to find the most similar one
> in the supported formats list
Not a bad idea here, but is it really worth the complexity? I noticed you have to go through the list of formats twice to do this. I wonder if something like a binary search tree or heap might work better here, but it's just a rough thought. 
> - if no format is set through gconf use best available format for the current
> webcam
> - avoid duplicates in the preferences dialog using "VideoFormat"
> (mimetype@widthxheight) items in the resolution combo
> 
> I think we should have per webcam settings in gconf since each device supports
> different formats. This would also allow to skip format detection for "known
> webcams" at startup saving a lot of time.
Nice idea, but I don't think GConf is the place for this. If you have a lot of different devices you're using at different times, you could really muck up the config database with a bunch of keys. What if we stored these things in something like an XML file? 

Also, I have some comments on style. For starters, you let some C++ comments get into your patch. Also, I noticed you use some less than descriptive variable names in a few functions, i.e. w, d, n, etc. Some of these I can tell what they are, but others are less obvious. Using a and b for parameters in your compare function is fine, as that's the usual convention for compare parameters, but as for local variables, I'd like to see more descriptive names like width, height, etc. Yeah, it's a matter of personal taste, but I really would like the code to be as self-documenting as possible. :)

And, I think this is very complicated. At the very least, we should try to find a way to simplify it. I don't have any concrete ideas right now, but let me think about it a bit. Otherwise, we could at the very least abstract all of this with some new GObject classes to represent video formats, and that would at least keep the webcam code a little cleaner. 

Thanks for looking into this.

>I don't think this is going to be the case forever, so assuming this is a bad
>idea. When I was designing the prefs system one of the things that Jaap wanted
>was the ability to select framerates. We also talked about the idea of
>selecting video formats too. Your solution would make implementing these very
>hard, and we'd probably have to come back and refactor it again 

First, there is a bug, second it has to be solved for 2.24.
I don't think we should care too much about the best way to do it now. The previous code looked for highest framerate, this what I've done, no regression. Anyway there is no need to refactor anything, take a look at cheese_webcam_lookup_format, now, if a better framerate is provided it replaces the current one, it's not so hard to save it into a list of framerates instead.
Besides I don't think that selecting framerate can be a desirable feature.. what's the point about selecting a low framerate?

>Not a bad idea here, but is it really worth the complexity? I noticed you have
>to go through the list of formats twice to do this. I wonder if something like
>a binary search tree or heap might work better here, but it's just a rough
>thought. 

Where is the complexity? What do we do now if a wrong format is selected through gconf? Going two times through the list doesn't happen so often, if a correct format is selected (supported mimetype, usual case) it has to go only once into the list and it's not so expensive IMHO (nothing compared to camera detection routines).

>Nice idea, but I don't think GConf is the place for this. If you have a lot of
>different devices you're using at different times, you could really muck up the
>config database with a bunch of keys. What if we stored these things in
>something like an XML file?

Sure, I agree, I said it in a comment to the duplicated resolution bug too. XML would be good.

>Also, I have some comments on style. For starters, you let some C++ comments
>get into your patch. Also, I noticed you use some less than descriptive
>variable names in a few functions, i.e. w, d, n, etc.

As I said I wrote it quickly, style was not my highest priority ;). About c++ comments, I usually remove them, I must have missed a couple of those, sorry.
Regarding w, h, n, d.. long names were already taken :)

>And, I think this is very complicated. At the very least, we should try to find
>a way to simplify it. I don't have any concrete ideas right now, but let me
>think about it a bit. Otherwise, we could at the very least abstract all of
>this with some new GObject classes to represent video formats, and that would
>at least keep the webcam code a little cleaner

I think is much more simple than before. A list is simpler to manage than a GArray, the code looks better, it can be sorted easily, there is no need for supported_resolution hashtable.
GObject doesn't always simplify things. Look at the preferences dialog code, it's just a 2 combo dialog that would have taken much less code without gobject, but there we have some "scalability" advantage, what will be the advantage here? 
Usually it makes things more complex adding a level of abstraction, I don't think a custom class it that needed for videoformats. Anyway it seems we have different ideas about complexity :P

(In reply to comment #6)
> >I don't think this is going to be the case forever, so assuming this is a bad
> >idea. When I was designing the prefs system one of the things that Jaap wanted
> >was the ability to select framerates. We also talked about the idea of
> >selecting video formats too. Your solution would make implementing these very
> >hard, and we'd probably have to come back and refactor it again 
> 
> First, there is a bug, second it has to be solved for 2.24.
> I don't think we should care too much about the best way to do it now. The
> previous code looked for highest framerate, this what I've done, no regression.
> Anyway there is no need to refactor anything, take a look at
> cheese_webcam_lookup_format, now, if a better framerate is provided it replaces
> the current one, it's not so hard to save it into a list of framerates instead.
> Besides I don't think that selecting framerate can be a desirable feature..
> what's the point about selecting a low framerate?
At the risk of sounding preachy, this line of thinking is very dangerous. Sure, we need to fix this problem quickly. However, it is a bad idea to assume too much when solving problems, and it seems to me that you're doing just that. As it so happens, I agree with you that I'd never change my framerate, but we can't say that for everybody that would ever come along. Maybe someone might have a really high-end HD camera or something that can do like 30-120 FPS or more and might need to adjust it; you never really know, and you can't really predict what kind of things users will want to do in the future. Making this kind of assumption is especially bad in this case, seeing as that such a feature has been asked for in the past, so I know at least one person wants it. See bug 522200 for instance. 

Besides, once this bug is fixed, where's the motivation to do this properly after that point? Gone. Therefore, it'd probably stay this way until someone decides they want to implement this feature. And because your solution assumed just a little bit too much, this makes that individual's job much harder than it really has to be.
> 
> >Not a bad idea here, but is it really worth the complexity? I noticed you have
> >to go through the list of formats twice to do this. I wonder if something like
> >a binary search tree or heap might work better here, but it's just a rough
> >thought. 
> 
> Where is the complexity? What do we do now if a wrong format is selected
> through gconf? Going two times through the list doesn't happen so often, if a
> correct format is selected (supported mimetype, usual case) it has to go only
> once into the list and it's not so expensive IMHO (nothing compared to camera
> detection routines).
What I mean here is that it adds a lot more functions to the already overly complicated cheese-webcam.c file. Besides, as we're in feature freeze, isn't this a little overreaching at this point anyway? 
> 
> >Nice idea, but I don't think GConf is the place for this. If you have a lot of
> >different devices you're using at different times, you could really muck up the
> >config database with a bunch of keys. What if we stored these things in
> >something like an XML file?
> 
> Sure, I agree, I said it in a comment to the duplicated resolution bug too. XML
> would be good.
> 
> >Also, I have some comments on style. For starters, you let some C++ comments
> >get into your patch. Also, I noticed you use some less than descriptive
> >variable names in a few functions, i.e. w, d, n, etc.
> 
> As I said I wrote it quickly, style was not my highest priority ;). About c++
> comments, I usually remove them, I must have missed a couple of those, sorry.
> Regarding w, h, n, d.. long names were already taken :)
> 
> >And, I think this is very complicated. At the very least, we should try to find
> >a way to simplify it. I don't have any concrete ideas right now, but let me
> >think about it a bit. Otherwise, we could at the very least abstract all of
> >this with some new GObject classes to represent video formats, and that would
> >at least keep the webcam code a little cleaner
> 
> I think is much more simple than before. A list is simpler to manage than a
> GArray, the code looks better, it can be sorted easily, there is no need for
> supported_resolution hashtable.
> GObject doesn't always simplify things. Look at the preferences dialog code,
> it's just a 2 combo dialog that would have taken much less code without
> gobject, but there we have some "scalability" advantage, what will be the
> advantage here? 
> Usually it makes things more complex adding a level of abstraction, I don't
> think a custom class it that needed for videoformats. Anyway it seems we have
> different ideas about complexity :P
Yes, it seems we do. :) What it appears to come down to is that we define "complexity" differently in this context. I call something "complex" if it does a lot of things in one place, especially if said things can be made independent of one another. What this does is break down very complicated tasks, like setting up those combos in the prefs dialog, into logical units that are intended to make the whole process more understandable at both the micro and macro levels. I designed the prefs system like this because, quite simply, it takes a fair bit of work to populate and manage those controls with the proper data with the constraints imposed on them. Had I put all of this into one file, I really think it would have been really hard not just to extend, but to really understand it and to maintain it because you have so much going on, and it sometimes becomes difficult to wrap your mind around all of that at once. With this modular design, readers can look at only the parts that interest them, and take a look at the dialog code to see how it all ties together in the big picture. 

I feel that the same could be done for the webcam code, and it would make it a lot easier to understand the individual parts. When I first wrote the resolution changing code, it took me at least a week to get the gist of how it works and where I needed to make modifications. IMHO that's entirely too long, and even after I was done with the patch I still wasn't too satisfied with the way it was implemented, because it just piled on an already complicated piece of code. 

I guess ATEOTD that's just how I like to write my code: in small, modular chunks, because in my experience that "scalability" advantage really is real. You can reuse a lot of code in different places and factor out common code to get rid of repeated code, and cut out some steps in the process of adding new features, among other things. For example, in the prefs code, the combo boxes are exceptions here because custom data models have to be maintained. Other things, like text boxes or check boxes, could just have one object to rule them all because they're relatively simple. I came up with this concept not just to solve the immediate problem, but also to help solve other problems down the road by keeping my design as flexible as possible, and I believe such a strategy will pay dividends when the time comes.

A few months ago, there was a post on the Cheese ML about making Cheese more modular in its design. See http://mail.gnome.org/archives/cheese-list/2008-June/msg00008.html for Daniel's comments on this. I find the fist paragraph to be most telling, where he seems to concede that some places, CheeseWebcam and CheeseWindow in particular, are more complex than they really should be, and that it is "quite hard to find bugs." 

Ok, not that I'm trying to give you a lecture or anything like that; I just want to give you a clear picture of my position, especially why I think hurrying to fix the immediate bug at the expense of extensibility, readability, and maintainability is a big mistake. 

I've seen the same problem in a much more extreme form with a bttv card, which supports 18 different rgb + yuv formats.

I've got a difference less invasive approach to fixing this, first of all we should not specify to gstreamer wether to use yuv or rgb, gstreamer can best choose this itself, I've got a patch attached for this (and some discussion about this) attached to bug 546868

Then the above list actually comes even more repetitious as there then is no difference between rgb and yuv formats too, so my plan is to in cheese_webcam_get_supported_video_formats add the same resolution only once, if there are multiple formats with the same resolution but with different framerates, choose the one with the highest framerate.

I've been working on this yesterday, but got sidetracked (fixing bugs in the gspca spca561 driver), so all I did yesterday is a patch shuffling some code in preparation for adding formats with the same resolution only once.

I'll attach this shuffle patch now, the plan is to check if a resolution is already added in cheese_webcam_add_video_format using the supported_resolutions hashtable, and if it i already added compare the framerates to see which one to keep.

Created attachment 117823 [details] [review]
Patch: shuffle some code in preparation for fixing the resolutions being listed multiple times issue

Note this patch should eb applied on top of my supported_resolutions per device patch from bug 546868

Created attachment 117873 [details] [review]
Patch: only list resolutions once

And here is the patch which actually implements the only listing resolutions once.

Created attachment 117874 [details] [review]
Patch: sort the list of resolutions

And here is one last patch sorting the list of resolutions, as having the resolutions in random order looks rather bad in the resolution selection combobox.

Created attachment 117960 [details] [review]
Patch: shuffle some code in preparation for fixing the resolutions being listed multiple times issue

New version against 2.23.91 (still needs patches from bug 546868 to be applied first) the other 2 patches still apply against 2.23.91

"Patch: only list resolutions once" and "Patch: shuffle some code in preparation for fixing the resolutions being listed multiple times issue" do not apply cleanly to current svn, would you please be so kind to revise your patches?

thanks a lot!

(In reply to comment #7)
> At the risk of sounding preachy, this line of thinking is very dangerous. Sure,
> we need to fix this problem quickly. However, it is a bad idea to assume too
> much when solving problems, and it seems to me that you're doing just that.

No assumption, I really think we should not let the user change framerate, and I still see no reason to use a lower framerate than the maximum supported.

> it so happens, I agree with you that I'd never change my framerate, but we
> can't say that for everybody that would ever come along. Maybe someone might
> have a really high-end HD camera or something that can do like 30-120 FPS or
> more and might need to adjust it;

I don't think human eye is able to perceive a difference between 30 fps and 120 fps (because of persistence of vision), so I don't think a device like this will ever exist, and no, I don't think it's such a big assumption.
Given that a device like this will be selled "in future" (marketing doesn't care about physiology) nobody would ever like to use it with lower framerate than the one advertised..
Anyway I think you should read Hans's comment about webcams and framerates on bug 522200. 

> you never really know, and you can't really
> predict what kind of things users will want to do in the future. Making this
> kind of assumption is especially bad in this case, seeing as that such a
> feature has been asked for in the past, so I know at least one person wants it.
> See bug 522200 for instance.

Sure, see that bug. That one is about framerate in recorded video.
There is a big difference between changing the framerate on video4linux src caps and changing it in the recording pipeline.
The former makes no sense to me, I think we should always set the maximum framerate supported by the devices on the videosource.

Recorded video framerate is an important parameter for video quality but, since live encoding with theoraenc can be quite expensive, it's a big parameter for recording performance too. A user might want to reduce it because recording at full framerate it's not affordable with his cpu and I think we should really provide an option for this. It's easily achieavable because we already have a videorate element in the recording pipeline (bug #542014) and it's just a matter of setting a property and adding an item to the preferences dialog.
This would not affect at all the real device framerate, it would just drop some frame to speed up encoding.

> 
> Besides, once this bug is fixed, where's the motivation to do this properly
> after that point? Gone. Therefore, it'd probably stay this way until someone
> decides they want to implement this feature. And because your solution assumed
> just a little bit too much, this makes that individual's job much harder than
> it really has to be.

A bit exagerating here? No so big assumption, not so hard to check old revisions and restore previous behavior.

> What I mean here is that it adds a lot more functions to the already overly
> complicated cheese-webcam.c file. Besides, as we're in feature freeze, isn't
> this a little overreaching at this point anyway? 

The feature is "add a preferences dialog to set the resolution", it already entered cheese before the feature freeze. Here we're trying to make it work properly.
Anyway, regarding complexity, we have a chance to reduce it a lot in cheese-webcam.c. 
Hans already removed the need to set the mimetype (we no more care about rgb or yuv and let gstreamer do it for us) in the videosource caps, I think we can also remove the framerate code since gstreamer can pick the maximum available for us. So most of CheeseVideoFormat code could be removed and simplified with these changes.

> Yes, it seems we do. :) What it appears to come down to is that we define
> "complexity" differently in this context. I call something "complex" if it does
> a lot of things in one place, especially if said things can be made independent
> of one another. What this does is break down very complicated tasks, like
> setting up those combos in the prefs dialog, into logical units that are
> intended to make the whole process more understandable at both the micro and
> macro levels. I designed the prefs system like this because, quite simply, it
> takes a fair bit of work to populate and manage those controls with the proper
> data with the constraints imposed on them. Had I put all of this into one file,
> I really think it would have been really hard not just to extend, but to really
> understand it and to maintain it because you have so much going on, and it
> sometimes becomes difficult to wrap your mind around all of that at once. With
> this modular design, readers can look at only the parts that interest them, and
> take a look at the dialog code to see how it all ties together in the big
> picture. 

That's what I meant with "scalability advantage".

> I feel that the same could be done for the webcam code, and it would make it a
> lot easier to understand the individual parts. When I first wrote the
> resolution changing code, it took me at least a week to get the gist of how it
> works and where I needed to make modifications. IMHO that's entirely too long,
> and even after I was done with the patch I still wasn't too satisfied with the
> way it was implemented, because it just piled on an already complicated piece
> of code. 
> 
> I guess ATEOTD that's just how I like to write my code: in small, modular
> chunks, because in my experience that "scalability" advantage really is real.
> You can reuse a lot of code in different places and factor out common code to
> get rid of repeated code, and cut out some steps in the process of adding new
> features, among other things. For example, in the prefs code, the combo boxes
> are exceptions here because custom data models have to be maintained. Other
> things, like text boxes or check boxes, could just have one object to rule them
> all because they're relatively simple. I came up with this concept not just to
> solve the immediate problem, but also to help solve other problems down the
> road by keeping my design as flexible as possible, and I believe such a
> strategy will pay dividends when the time comes.

Agree, but often this strategy can complicate tasks (in the sense of increasing code amount and abstraction) otherwise much more simple. It works well with UI things, where the object model really fits but it can complicate backend ones (like cheese-webcam) adding abstraction where it's needed to have a clear view of the sequence of actions we're doing. It's not easy to explain well what I think, I agree with your thinking but I don't believe it really fits this case.

> A few months ago, there was a post on the Cheese ML about making Cheese more
> modular in its design. See
> http://mail.gnome.org/archives/cheese-list/2008-June/msg00008.html for Daniel's
> comments on this. I find the fist paragraph to be most telling, where he seems
> to concede that some places, CheeseWebcam and CheeseWindow in particular, are
> more complex than they really should be, and that it is "quite hard to find
> bugs." 

See what I say above. CheeseWebcam is too much complex because we do a lot of work that gstreamer could do for us.


Marking my patch as obsolete because of recent changes from bug 546868.
Comment on Hans patches following.

(In reply to comment #10)
> Created an attachment (id=117873) [edit]
> Patch: only list resolutions once
> 
> And here is the patch which actually implements the only listing resolutions
> once.

Having two data structures (the array and the hash table) is redundant. You are using the hashtable as an interface to the array, why not to use just the hashtable (is it sortable in a easy way?) or implement a lookup function for the array? what about using a list (as you can see above I like them more than garrays..)?

Just to clarify things: this patch does not solve this bug. It could solve bug #547140. This bug is about duplicated items in the videoformat array, you now have unique formats in the hashtable but the array still contains duplicated entries (if I'm not missing anything in your code). Maybe it has no noticeable effect for the user but I don't think we should save those formats.
What you/we're doing now is save all those duplicated formats in the array and filter them, remove duplicates with the hashtable... I think we should not save them at all.
Also I think we should not save all supported framerates if we're going to use the maximum one only. (As I said before, there is no need to set it since gstreamer already picks the maximum one if nothing set).
So CheeseVideoFormat could be reduced to contain just width and height simplifying a lot the whole CheeseWebcam thing.

There is still an issue if the current device does not support the saved resolution. If an unsupported resolution is set through gconf or the current saved resolution is not supported by the current device we should try to find the nearest one within supported ones (see my patch above), or we should start to think about a per-webcam setting system (I think the former is probably easier to achieve in the short term).

(In reply to comment #13)
> "Patch: only list resolutions once" and "Patch: shuffle some code in
> preparation for fixing the resolutions being listed multiple times issue" do
> not apply cleanly to current svn, would you please be so kind to revise your
> patches?
> 

You mean the new version against 2.23.91 ?? You guys really should stop running indent scripts all the time, that is what broke my patches from 2.23.90 -> 2.23.91 I guess that has happened now too. Anyways I'm at a FOSS conference this whole weekend, so I don't have time for this ATM, it would be nice if someone  else could resolve the conflict.

(In reply to comment #15)
> (In reply to comment #10)
> > Created an attachment (id=117873) [edit]
> > Patch: only list resolutions once
> > 
> > And here is the patch which actually implements the only listing resolutions
> > once.
> 
> Having two data structures (the array and the hash table) is redundant. You are
> using the hashtable as an interface to the array, why not to use just the
> hashtable (is it sortable in a easy way?) or implement a lookup function for
> the array? what about using a list (as you can see above I like them more than
> garrays..)?
> 

The having 2 interfaces thing is from the existing code, not from my patch, I merely moved the populating of the hashtable earlier so that the hashtable can be sued to check for duplicate resolutions.

As for implementing a lookup function, thats exactly what the hashtable is used for, to lookup formats based on resolution nothing more and nothing less.

> Just to clarify things: this patch does not solve this bug. It could solve bug
> #547140. This bug is about duplicated items in the videoformat array, you now
> have unique formats in the hashtable but the array still contains duplicated
> entries (if I'm not missing anything in your code).

You are missing something in my code, new entries only get added to the array if they are not duplicate, so it does fix this bug.

> There is still an issue if the current device does not support the saved
> resolution. If an unsupported resolution is set through gconf or the current
> saved resolution is not supported by the current device we should try to find
> the nearest one within supported ones (see my patch above), or we should start
> to think about a per-webcam setting system (I think the former is probably
> easier to achieve in the short term).
> 

I would prefer to see this fixed by saving the resolution per device, for example for my tvcard to work I must select a resolution of 388x244 (or something like that), the framegrabber part of the TV-card can do higher resolutions, but the tuner delivers a video signal of this resolution so then the grabber won't lock. When I plugin a webcam which is capable of both 352x288 and 640x480, I would like it to use 640x480 not 352x288, so the resolution setting should clearly be per device and then there is no reason whatsoever to complicate the code by looking for a nearest resolution.

Created attachment 118148 [details] [review]
Patch: shuffle some code in preparation for fixing the resolutions being listed multiple times issue

updated patch to apply to current svn

Created attachment 118149 [details] [review]
Patch: only list resolutions once

updated patch to apply to current svn

(In reply to comment #17)
> The having 2 interfaces thing is from the existing code, not from my patch, I
> merely moved the populating of the hashtable earlier so that the hashtable can
> be sued to check for duplicate resolutions.

I know that's from existing code no blame for you! It's just a chance to do it better.

> As for implementing a lookup function, thats exactly what the hashtable is used
> for, to lookup formats based on resolution nothing more and nothing less.

Sure, I see. That's the reason I suggested to use only one data structure, either the hashtable only or the array. GArray, though, has no lookup function so a new one would be needed.
 
> You are missing something in my code, new entries only get added to the array
> if they are not duplicate, so it does fix this bug.
Yup :) you're right. I didn't noticed it yesterday (too tired after too much driving). Now that I can apply it to svn I see what you're doing, sorry.
 
> I would prefer to see this fixed by saving the resolution per device, for
> example for my tvcard to work I must select a resolution of 388x244 (or
> something like that), the framegrabber part of the TV-card can do higher
> resolutions, but the tuner delivers a video signal of this resolution so then
> the grabber won't lock. When I plugin a webcam which is capable of both 352x288
> and 640x480, I would like it to use 640x480 not 352x288, so the resolution
> setting should clearly be per device and then there is no reason whatsoever to
> complicate the code by looking for a nearest resolution.

Ok, we'll work on it. I doubt it could be ready for 2.24 though.

(In reply to comment #19)
> Created an attachment (id=118149) [edit]
> Patch: only list resolutions once
> 
> updated patch to apply to current svn
> 

Ops, this patch also contains the code shuffle one :) I'll update it if needed, but I think it can be reviewed all at once too.

Created attachment 118150 [details] [review]
Patch: only list resolutions once

Fix the patch to not contain the code shuffle one.

(In reply to comment #16)
> (In reply to comment #13)
> > "Patch: only list resolutions once" and "Patch: shuffle some code in
> > preparation for fixing the resolutions being listed multiple times issue" do
> > not apply cleanly to current svn, would you please be so kind to revise your
> > patches?
> > 
> 
> You mean the new version against 2.23.91 ?? You guys really should stop running
> indent scripts all the time, that is what broke my patches from 2.23.90 ->

we just did that once after the 2.23.90 release ;)

> 2.23.91 I guess that has happened now too. Anyways I'm at a FOSS conference
> this whole weekend, so I don't have time for this ATM, it would be nice if
> someone  else could resolve the conflict.
> 

all patches are ok by me. if you guys want them into 2.24 we should apply them till tomorrow, monday.

Ok, closing this bug, all patches committed to svn trunk.
I still don't like to have 2 data structures for the same thing and probably cheese-webcam still needs some work, but we have to postpone it to 2.26.