[Bug 735660] v4l2: fix new v4l2 code not working with certain devices (regression)

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=735660
  GStreamer | gst-plugins-good | 1.4.1

--- Comment #13 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2014-08-29 19:53:24 UTC ---
(In reply to comment #11)
> (In reply to comment #7)
> > Note the other patch seems incomplete fixed around bug 726195 and 726194. We
> > need to write proper interlacing detection code, it's not true that for 1
> > resolution driver only do mixed or progressive.
> > Would it be possible to move patches to proper bugs instead of having all unrelated issue in the same bug ?
> 
> Note the subject of this bug: "v4l2: fix new v4l2 code not working with certain
> devices (regression)", so if you wonder why all the patches are together in a
> single bug, because the entire set is needed to get things work again / to fix
> the regression.
> 
> Frankly, I'm quite unhappy about all the flack I'm getting here, don't shoot
> the messenger and all that. I just spend a couple of hours this morning to fix
> a regression I did not cause, and instead of a thank you I'm getting a lot of
> push-back, and no indication of any willingness to actually care about and fix
> the regression. Not a great way to win harts and minds IMHO.
> 
> No about the technical argument you make:
> 
> > Note the other patch seems incomplete fixed around bug 726195 and 726194. We
> > need to write proper interlacing detection code, it's not true that for 1
> > resolution driver only do mixed or progressive.
> 
> Yes some devices may support both progressive and interlaced for a single
> resolution. But currently the code is not structured to deal with this, dealing
> with this requires at least some refactoring. My patch is not about this, my
> patch is a bug fix to the current code, which follows a model where only one of
> progessive or interlaced can be supported, and tries to get progressive first.
> 
> This current code, which is currently shipping in a stable release, is broken
> not at a design level which you're talking about now (it could use improvements
> there too). but in the implementation of the chosen design. Even following the
> chosen design of a mode being either progressive or interlaced it get things
> wrong, with as end result that things do not work at all.
> 
> So rather then talking about design issues, which seems to me to be something
> for 1.5.x, lets first focus on fixing the regression (ah there is that word
> from the summary again), and get the current code to at least work properly for
> what it does do, and that is *exactly* what my patch does.

I'm not making an argument against your work, I really appreciate that you take
the time to report and propose patches that fixes issue you have. This is blind
review, and request for comment. I only rejected the third patch because it
breaks (yes literally) other use case I'm aware of. The two other patches may
be merged when I'm done going over all use cases I know (that includes trying
to see if they help with related bugs, or would lead to more re-write later).

The reason it takes a lot of time and effort to get stuff in for v4l2 is that
we really need to dig all patches deeply as everyone writes patch for their
single HW and assume it's the absolute solution.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.