On Tue, 8 Mar 2022 17:36:47 +0100 Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > Hi Pekka, > > On 3/8/22 15:30, Pekka Paalanen wrote: > > On Tue, 8 Mar 2022 13:09:37 +0100 > > Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > > >> Hi Shashank, > >> > >> There is no cover letter for this series, so I'll just reply to the > >> first patch, but my comments are high-level and not specific to this > >> patch. > >> > >> To be honest, I am not at all convinced that using edid-decode as a > >> parser library is the right thing to do. It was never written with that > >> in mind. > > > > Hi Hans, > > > > in https://www.spinics.net/lists/linux-media/msg190064.html you wrote: ... > >> I think edid-decode can function very well as a reference source for > >> a real EDID parser since edid-decode is very complete, but not as a > >> EDID parser library. > > > > It would be a shame to have to fork edid-decode into something else and > > then play catch-up with the real edid-decode for all times to come. Or > > are you perhaps hoping that the fork would eventually completely > > supersede the original project and developers would migrate to the new > > one? > > > > It would be really nice to be able to involve the community around > > edid-decode to make sure we get the library right, but if the library > > is somewhere else, would that happen? Or are we left with yet another > > half-written ad hoc EDID parsing code base used by maybe two display > > servers? > > > > Maybe we could at least work on this proposal for a while to see what > > it will start to look like before dismissing it? > > If you are willing to put in the effort, then I think you would have to > first rework the code bit by bit into different layers: Hi Hans, thanks! If Shashank agrees, we can see how this would start to look like. I suppose there would be the occasional patch series sent to this mailing list and publicly discussed between me and Shashank while we iterate. You could mostly ignore it if you want until the two of us need your guidance. Even if it turns out that this cannot go into edid-decode upstream, I don't think the exercise is going to go to waste. It would be the beginnings of a new project. > E.g. parse_base_block() would be split into two functions: a parse_base_block() > that parses the base block into C structures, and it also does the conformity > checks, where the output of that is just written to an internal buffer, as > happens today. The --check-inline option functionality would be hard to support, > I suspect, but I think it is OK to drop that. I at least rarely use it. For --check-inline, maybe, maybe not. open_memstream() is a thing, giving us a FILE*. Depending on --check-inline, the FILE* to complain to could be either stderr or an internal buffer from open_memstream(). Or the C++ equivalent. > And on top of that there is a print_base_block that produces the human > readable output based on the result of the parse_base_block. > > Later the parse functions can be put in a library which edid-decode uses. > > It should be possible to do this conversion bit by bit, so it's easier to merge > and maintain. > > But it is a *lot* of work since you will also have to make C headers for all > the EDID structures. Thank you for the suggestions and warnings. I suspect we shouldn't aim to land the first part until we have a good idea of the later parts, so that edid-decode does not end up with half a conversion if the later parts turn out too messy. > Can the library be C++ or do you need C structs only? If C++ is OK, then that > will simplify matters. The only thing that absolutely must be C is the library public API. What I've been imagining so far is that we would have a low-level and a high-level API, as I alluded to in my previous email. Other than that, I don't know yet. Internals are totally fine to keep as C++. > In any case, I think I would like to see a proof-of-concept where the base > block parsing is modified in such a way as I described above. If that makes > sense, then this can be extended to the other extension blocks. And for the > CTA and DisplayID extension blocks you can probably do the conversion one > Data Block type at a time. > > In any case, this series is just not useful as proof-of-concept. I agree. A cover letter to set up your expectations would have been in order. Btw. how does edid-decode regression testing work? I thought I asked in the past, but I can't find my question or answer. I know what edid-decode README and test/README says about it, but how does one actually run through the tests? One thing I'm a little wary of is the edid-decode.js target in the Makefile. How do you test that? On the other hand, if merging into edid-decode does not work, a new project could have several benefits if I get to decide: - Meson build system - automated test suite in the project - Gitlab workflow hosted by freedesktop.org - CI I must admit it is really tempting, but I'm scared of the amount of work needed to establish a new project. I assume you are not interested in any of that in the current upstream project, are you? Thanks, pq
Attachment:
pgp52n0rjhsoo.pgp
Description: OpenPGP digital signature
