Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API design? #2

Open
ppaalanen opened this issue May 6, 2021 · 34 comments
Open

API design? #2

ppaalanen opened this issue May 6, 2021 · 34 comments

Comments

@ppaalanen
Copy link

To me the API here looks very low-level and very rigid. When new EDID fields get standardized, how will you be able to extend this library without breaking its API or ABI?

When I look at your API, I cannot figure out how do I find the monitor make, model, and serial number. I would expect a library to be a lot easier to use.

I have a very naive view of EDID, but here is a counter-proposal for an API:

  • a function to create an opaque object from a raw EDID data, which all other functions operate on
  • low-level API specific to base block or an extension: a) does it exist? b) give me a structure with all the data in natural C types if it exists.
  • high-level API, e.g. "get me the screen physical dimensions", not necessarily covering everything the low-level API can provide

By "natural C types" I mean that e.g. strings are nul-terminated const char*, enums are converted into their represented values, e.g. float and so on. If EDID spec says that this unsigned 8-bit value is screen height in 0.5 cm units, then you'd return float height_cm or int height_mm. Things that are easy to understand in an API, not how they are encoded in EDID binary.

The high-level API example "get me the screen physical dimensions" is completely made-up, so humor me here even if what I'm saying next is not how physical size is actually stored.

Physical dimensions are returned from an API function as millimeters. EDID base block contains it in centimeters, so that needs to be converted to millimeters. However, the centimeter values could also be bogus, telling only the screen aspect ratio and not actual size, in which case size should not be returned at all. There could also be an extension giving more precise physical size, in which case the function needs to return that data, not the base block data. Getting aspect ratio needs a different function and will return the correct result even if physical size is stored properly, in base block and/or in extension, but it can also return the aspect ratio from the bogus size values when that is appropriate.

IOW, the low-level API would be for people who know that they want to look at the specific extension block, regardless of what else is in the EDID. The high-level API could internally use the low-level API, but it would also use the best common practices to figure out the actual answer than just telling that this EDID encodes physical size in three different ways and they all give a different result.

The high-level API could even include things like translating a four-character vendor code into a proper vendor name, if that makes sense.

Last, there is no need to prioritize parsing video mode information. For all Wayland display server purposes, we get all video mode resolution and timing information from KMS, and in fact we are told to not look at EDID because the kernel will adjust the mode list.

@contactshashanksharma
Copy link
Owner

Hello Pekka,
Thanks for your comment.

Yes, I understand that how this API looks low level, as I was working on the aspect that this library API will be consumed by the compositor, which knows what exactly it needs from it. But Now I am starting to see that how that might not be a very fair expectation.

As of now, I have extracted all the data, and positioned in form of a single API, but I liked the idea of multiple high level APIs running on top of this low level API, which will give specific information about the edid, for example:

is_monitor_hdr() ?
is_monitor_hdmi_2() ?
get_monitor_dimentions_mm()
give_me_static_hdr_metadata(),
give_me_the_preferred_mode()
give_me _the_monitor_dimentions()
does_monitor_support_scrambling()
get_me_supported_colorspaces()
get_me_supported_colorformats()
get_max_luminance()
get_min_luminance()

I would love to hear a bit more about what could be a typical requirement or expectations from a compositor side, like listed above. I will start working on phase two of the library sooner,

  • Shashank

@ppaalanen
Copy link
Author

Indeed. Compositors more or less know what they want, but not down to the level where you need to know the EDID specification by heart to be able to operate the library API. It is good that the API documentation refers to EDID specs for the authoritative detailed truth, but I think we need something higher-level too. After all, the point of the library is to share the code that otherwise every compositor would need to write themselves. The low-level API I suggested then allows someone to deviate from the usual logic, heuristics or quirk-fixups, if they want to.

Looking at Weston, it currently uses find_and_parse_output_edid() to get the monitor make, model and serial number, if they exist.

Furthermore, Weston gets the physical size from the kernel. I don't know if that has any caveats, but generally we should use what the kernel exposes, and parse EDID only as a fallback.

The CM&HDR work is going to need:

  • color characteristics
  • gamma? I'm not sure if that is actually useful.
  • colorimetry data block?
  • HDR static metadata

I got that list by looking at what edid-decode says about my HDR monitor.

Another useful thing could be a hash value of the raw EDID data. When EDID lacks serial number, I suppose the EDID data itself could be used to identify a monitor up to some degree - hopefully the user does not have two monitors with identical EDIDs. What hash algorithm to use, I'm not sure. I would claim it does not need to be cryptographically sound, but it needs to be good enough to tell monitors apart, so I wouldn't pick CRC32 I guess. However, one could also make the point, that compositors likely needs hashes of other things, too (e.g. ICC files), so they can just compute it themselves for their own purposes.

@contactshashanksharma
Copy link
Owner

contactshashanksharma commented May 8, 2021

Ok, I think we have enough information to start with the high level APIs. We can always add another API on request / need / feature addition.

About the hashing of the RAW edid, I am not very sure about it, as it seems out of the scope of this library. As we can see that we get the EDID from DRM connectors, which are filled by display driver, which are directly accessible to the compositor as well. If required, I guess a better approach would be to add an API which can extract RAW EDID and it's size from the DRM node, and pass it to compositor, and it can do it by itself, using whichever method/API supported.

@ppaalanen
Copy link
Author

This library should have no calls to KMS or libdrm, IMO.

Yes, hashing is likely out of scope.

@jnikula
Copy link

jnikula commented May 10, 2021

What happened to the idea of splitting edid-decode to a library and a cli frontend?

@ppaalanen
Copy link
Author

I'm just going by whoever wants to do the work. @contactshashanksharma ?

@jnikula
Copy link

jnikula commented May 10, 2021

Just looking at the code bases, edid-decode is much more feature complete and battle tested. I am not sure why we would want to start over, and end up with two parsers.

@contactshashanksharma
Copy link
Owner

@jnikula : Hello Jani, I am planning to extend this library, due to the following reasons:

  • edid-decode is a standalone app, which is written to serve a different purpose. I want to write something which is completely designed to help a compositor in mind. All the APIs would be written in such a way which could be directly embedded into a compositor, while it makes it decision about compositon.
  • Also, the focus is more towards browsing advanced features for modern displays which are exposed by CEA blocks, instead of focusing the base blocks only.
  • Once we have this in form of library, any application like a compositor/ an IGT/ a display server can use this and unitilize this in it's context.

@contactshashanksharma
Copy link
Owner

This library should have no calls to KMS or libdrm, IMO.

Yes, hashing is likely out of scope.

Separation from libdrm can be very easily done, but i was thinking from a compositor point of view. In general, I was thinking of providing APIs so that a compositor just has to pass a DRM connector, and the library itself can extract the EDID from the connector (or all the connectors on a card). Else the compositor will have to first extract the EDID from conenctor, and then give that to the library call.

In fact, we can implement both. I was thinking one basic API, which parses the raw EDID, and one API on top on it, which extracts the EDID from DRM connector. If you do not want to utilize the DRM API, you can directly get the EDID and pass it to lower level API.

@ppaalanen
Copy link
Author

I believe this library must not assume the caller is using DRM or KMS, there is no reason why this library should depend on libdrm. The purpose is purely to convert EDID data into usable information, just like edid-decode does except in a way usable from C. That conversion is the difficult part. Integration with libdrm is trivial and likely not worth sharing.

See e.g. https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/102#note_869969 for a reason why one wouldn't use a helper function.

Therefore I don't quite see why this wouldn't fit with the edid-decode code base. Can you expand on that?

@jnikula
Copy link

jnikula commented May 11, 2021

Long term, I don't think it makes sense to have more than one EDID parsing library. The purpose is to parse EDID, and I don't think it should matter what the parsed information is used for, displaying the info or compositor or what. We're going to need all of those use cases.

I'll list some of the things edid-decode has going for it, and why I think it would be benefitial to refactor the library out of it, instead of writing from scratch:

  • Arguably, it's the most feature-full parser we have at the moment.
  • It is widely used, and has seen a lot of real world EDIDs.
  • It supports DisplayID extension blocks.
  • It has a long history, and a small community around it already.
  • It is readily packaged to a lot of distros.
  • It is possible to carve the library out of it incrementally.
  • If you hit parsing issues with the compositor, you can debug them using edid-decode the tool, with the same parser.
  • You are going to need a cli frontend for debugging and testing no matter what. It's a use case you need to consider.
  • With a separate library you'd have an uphill battle for migrating edid-decode to use it and be the frontend.

Doing this from scratch without considering the legacy around it always has a certain appeal to it that I can definitely understand. It's fun to write a parser. It's fun to design an API from scratch. But it grows old pretty quickly, and you really want to have all the synergy you can get from working on the same code base, with as much help as you can get, implementing new features and fixing bugs in the same place only once. Even sending patches to edid-decode to make the changes is sure to provoke review comments in a way that's hard to solicit for a rewrite.

@contactshashanksharma
Copy link
Owner

Honestly, It's not just about fun of designing the API, but rather it's also about:

  • The time and energy we will have to invest just to convince the legacy stakeholders just to give a try to this idea. And beyond a certain time, it falls into one person, who wants to drive this initiative. I am personally not willing to do that, but if someone else wants to take that responsibility, I would be happy to join.
  • Based on my understanding of the code, EDID decode was always written to be a standalone tool, and it would need many design changes at the core, to converge that into a library. I am not sure how to convince someone on "why do you want to do that " ? I hardly think people will agree on taking additional responsibility of maintenance and testing just because some compositor wants to use it.
  • As a matter of fact, as it's widely used and regarded, this becomes a problem for us to convince the maintanres (you would understand that better than most of us).

I understand that your suggestion is very valid, from the reusability of trustworthy code point of view, but in my opinion it would be worthy to write a new library this time, as I am not very sure if I would be able to invest that much of time to convince people even before starting the work.

@ppaalanen
Copy link
Author

The time and energy we will have to invest just to convince the legacy stakeholders just to give a try to this idea.

I think that effort will be much easier if we build on/into edid-decode rather than start from scratch. We already have buy-in too, in that people do want a library for EDID parsing, so now it's just about convincing them about the technical quality of the implementation and the API.

Can you explain what exactly you intended to sign up for here? If you're not going to be the maintainer of the library, then that is even more reason to piggy-back on edid-decode.

Based on my understanding of the code, EDID decode was always written to be a standalone tool, and it would need many design changes at the core, to converge that into a library. I am not sure how to convince someone on "why do you want to do that " ? I hardly think people will agree on taking additional responsibility of maintenance and testing just because some compositor wants to use it.

Did you see the response from Hans Verkuil in the email thread I started? He sounded very positive about carrying this in edid-decode, so I don't think this is much of a concern. It may take work, but you wouldn't need to fight for the "why".

Working on edid-decode shouldn't be much different than what you started with here:

  • Figure out a build system to create a versioned shared library with no code.
  • Make draft of the API people would like for starters.
  • Plan with the maintainers how edid-decode code should change to be able to implement that API.
  • Implement the API and send patches to edid-decode upstream.
  • Revise patches based on review comments until they land.
  • Repeat for the next piece of API.

In my mind, going with edid-decode is the easy path given that they already said yes to the idea.

I also saw that edid-decode repository does not have any testing scripts, so I don't know how they test the code. But if edid-decode itself uses the library API, then the (static copy of the) library gets tested on the side.

@contactshashanksharma
Copy link
Owner

The time and energy we will have to invest just to convince the legacy stakeholders just to give a try to this idea.

I think that effort will be much easier if we build on/into edid-decode rather than start from scratch. We already have buy-in too, in that people do want a library for EDID parsing, so now it's just about convincing them about the technical quality of the implementation and the API.

Can you explain what exactly you intended to sign up for here? If you're not going to be the maintainer of the library, then that is even more reason to piggy-back on edid-decode.

My intention is to maintain this library, and keep on adding features into this.

Based on my understanding of the code, EDID decode was always written to be a standalone tool, and it would need many design changes at the core, to converge that into a library. I am not sure how to convince someone on "why do you want to do that " ? I hardly think people will agree on taking additional responsibility of maintenance and testing just because some compositor wants to use it.

Did you see the response from Hans Verkuil in the email thread I started? He sounded very positive about carrying this in edid-decode, so I don't think this is much of a concern. It may take work, but you wouldn't need to fight for the "why".

Would you mind to start this discussion in their mailing list, so that we can get an idea of how the community thinks ? It would only make sense if people think its a good idea, and for that, we should check the genral feeling around it. I don't really want to embed a lot of code, and then realize that people are not ready to take that in.

  • Shashank

Working on edid-decode shouldn't be much different than what you started with here:

  • Figure out a build system to create a versioned shared library with no code.
  • Make draft of the API people would like for starters.
  • Plan with the maintainers how edid-decode code should change to be able to implement that API.
  • Implement the API and send patches to edid-decode upstream.
  • Revise patches based on review comments u.ntil they land.
  • Repeat for the next piece of API.

In my mind, going with edid-decode is the easy path given that they already said yes to the idea.

I also saw that edid-decode repository does not have any testing scripts, so I don't know how they test the code. But if edid-decode itself uses the library API, then the (static copy of the) library gets tested on the side.

@ppaalanen
Copy link
Author

I thought I already did start that email thread with the edid-decode community:
https://lists.freedesktop.org/archives/wayland-devel/2021-April/041763.html

You can also find the thread in linux-media archives:
https://www.spinics.net/lists/linux-media/msg190062.html

@ppaalanen
Copy link
Author

You can also find it on dri-devel:
https://lists.freedesktop.org/archives/dri-devel/2021-April/302480.html

And xorg-devel:
https://lists.x.org/archives/xorg-devel/2021-April/058690.html

I think that covers the intended audience quite well. There are comments from GNOME, Enlightenment, wlroots, even Mir. I would expect KDE to welcome it as well.

We didn't get a reply specifically on behalf of Xorg, but that's also not a surprise given the project is in maintenance-only mode. I think replies are showing an exceptionally positive reception to a proposal of a shared library.

Whether people assumed it would be growing out of edid-decode or not I do not know, but I think it would be a safe bet.

@contactshashanksharma
Copy link
Owner

Ah, that's good. Let me go through the mail chains, and understand and interpret the intent and inclinations, so that I can start exploring edid-decode.

@nwnk
Copy link

nwnk commented May 12, 2021

I've had this tab open for about a week intending to comment, probably I should do that...

I will say that factoring EDID handling out to a library is like right on the line of whether it's worth it or not, in my experience. There's kind of two things you want to do with EDID, either you want to ask very specific questions about the presence and value of a particular subfield or you want some kind of fuzzy interpretation like "what's the physical size". For the former things the parsing is just not that hard and you probably only need one or two such answers so it's really tempting to open-code. For the latter things it's hard to express even those questions in an extensible way and it's not always clear that different display servers would want the same interpretation. Imagine trying to ask "what's the best HDR standard the monitor supports" in a sufficiently future-proof way that an EDID extension could add a new standard and the app would know how to handle that answer... and now try to encode that into a C API in a way that isn't just adding more entrypoints with every release...

This was part of why I wrote edid-decode as a standalone validator. I wanted a reference parser to tell me every little detail and complain about every quirk, and then I wanted the interpreter in xserver to maybe be a bit more forgiving and only interpret the pieces it cares about. Which, for xserver, is mostly mode timings and limits, which maybe need quirking for some models but doesn't really need much interpretation based on the EDID itself. And using even that mode list requires some amount of post-processing since there's also the GPU device's limits to consider and you can't discover those from EDID (if you can discover them at all, usually you just have to know that Ironlake's VGA DAC limit is 250MHz or whatever).

I'd still say start with edid-decode for this, and only partly because it's my baby, Hans has done a great job with maintaining it and it's certainly the most complete open parser you're likely to find. I would probably focus my API design first around replacing existing hand-coded bits in weston/xserver/mutter/... so that it starts out as immediately usable, and generalize from there.

@ppaalanen
Copy link
Author

Thanks Adam, it seems we pretty much agree except maybe on the worthyness where I am much more optimistic. Your comment also validates my feeling that we need both a low-level and a high-level API. I very much agree on the suggested attack plan.

I don't see adding more entry points with every release as a big problem here, because the scope of the library is very well defined and it has no other inputs than an EDID (or DisplayID) blob, and no state to maintain.

I might even say that Xorg is not in the target audience of the library, exactly because Xorg wants modeline information from the EDID and I suspect that all Wayland compositors do not, and also because there is no benefit in migrating Xorg over. Xorg likely won't get new features anymore that would need new things from EDID.

@nwnk
Copy link

nwnk commented May 12, 2021

Thanks Adam, it seems we pretty much agree except maybe on the worthyness where I am much more optimistic.

Apologies if I came across as discouraging, it wasn't really my intent. I was more saying why I'd gotten discouraged before, as opposed to asserting that it's impossible or undesirable. By all means, make it happen, I think it'd be great.

I don't see adding more entry points with every release as a big problem here,

To the extent that this is like libdrm and we just assume eternal forward compatibility, that should be fine. I'm just thinking of the packaging problem for an app using this library to be able to require a new-enough version at runtime. It might make sense to have like a unsigned int libedid_version(void) that returns a sequence number for API additions so a careful app can strategically dlsym.

Xorg wants modeline information from the EDID and I suspect that all Wayland compositors do not

Mmmm. I can think of uses for userspace reconstruction of modelines from EDID. For example if the kernel's mode list for a connector doesn't include some things that are clearly present in EDID you can maybe guess that your source hardware needs a newer version of DisplayPort or dual-link DVI support or etc., which would be nice from a UI perspective for the display control panel to be able to tell you. Or, that your kernel's EDID parser doesn't support some new extension block.

And, if we're taking the strategy of making edid-decode itself consume this library, you're gonna need modelines.

@jnikula
Copy link

jnikula commented May 12, 2021

Mmmm. I can think of uses for userspace reconstruction of modelines from EDID. For example if the kernel's mode list for a connector doesn't include some things that are clearly present in EDID you can maybe guess that your source hardware needs a newer version of DisplayPort or dual-link DVI support or etc., which would be nice from a UI perspective for the display control panel to be able to tell you. Or, that your kernel's EDID parser doesn't support some new extension block.

I think we've pretty much decided the userspace should not use the modes directly from EDID, but rather rely on the modes exposed by kernel. eDP Multi-SST Operation (MSO) is one example where the EDID has the timings for a segment, while the panel consists of multiple segments. You should use the mode from kernel, not try to figure it out yourself. Of course, if the intent is to provide more info for the user, it's fine.

@ppaalanen
Copy link
Author

@contactshashanksharma Any news?

I might be starting to need EDID colorimetry and HDR information in a few weeks, and I'd like to have a plan on what to do about it. E.g. if I were to merge EDID parsing code in Weston, how likely am I going to have to live with it for a long time before replacing it with some form of libedid.

@contactshashanksharma
Copy link
Owner

contactshashanksharma commented Jun 28, 2021

@ppaalanen : Hello Pekka, I started analyzing the edid-decode code and started making the mental layout of the transformation layer. As of now It's been going at a (very) slow pace as I am not very verse in C++, and I also am in a 3 week vacation. It might be difficult to give some something near-reliable in July from my end. I might be able to present something around late August . I hope that would not be too late for you.

@ppaalanen
Copy link
Author

Sounds fine to me. I was interested if you were still working on it and whether you think anything would come out of it at all. If we might have something testable this year, that would be really nice and I can not worry about the quality of the EDID parsing code we might need in Weston the mean time for experiments.

@ppaalanen
Copy link
Author

@contactshashanksharma hi, any news on your effort?

@contactshashanksharma
Copy link
Owner

@ppaalanen : I just reached Berlin in Jan first week, settling down. Will resume work soon, and send any updates in the mailing list.

  • Shashank

@contactshashanksharma
Copy link
Owner

Hello @ppaalanen : I have sent the first set of patches for libedid-decode to the linux-media mailing list (keeping you and Jani in cc). Please check that out let me know how does it look.

As of now i have just implemented 7-8 fundamental APIs and a simple test utility to showcase the usages. Once the community gives its feedback, I will add complex APIs like color management and HDR.

@jnikula : FYI

  • Shashank

@contactshashanksharma
Copy link
Owner

@ppaalanen : I have updated V2 of the library, this is an updated version which addresses some of the API design issues you listed. This version introduces 28 new APIs to extract information from EDID. It also returns a transparent ptr. Please have a look if this seems something to be picked for new project.

@contactshashanksharma
Copy link
Owner

test-libedid-api.c shows the simple demo usages of APIs.

@swick
Copy link

swick commented Mar 23, 2022

Thanks for working on this!

Renaming this to libdisplay-info like pq suggested would be a good idea and would give us the di_ namespace.

I would also like to move this to the fdo gitlab, move from Makefile to meson and add CI.

For testing we should pull a database of EDID files from one or multiple sources (specially prepared ones and from actual displays) instead of hardcoding some EDIDs in C files.

One thing I don't like is that this assumes the only EDID extension is CTA. The other important one is DisplayID but there might be more.

The error handling in libedid_process_edid_info is not great. Maybe an opaque context struct might be useful, maybe just a parse error struct as return parameter.

pq also had some ideas regarding forwards and backwards compatibility.

The high level API might require information about what mode the display is in including active AVI InfoFrames. Maybe a struct dl_display_state that can be set up once and then passed to the high level API calls along with the EDID info.

This is a great start!

@contactshashanksharma
Copy link
Owner

contactshashanksharma commented Mar 23, 2022

Thanks for working on this!

Renaming this to libdisplay-info like pq suggested would be a good idea and would give us the di_ namespace.

I would also like to move this to the fdo gitlab, move from Makefile to meson and add CI.

Agree, this was suggested by Pekka as well. I thought that it would be a good idea to project a new version here, just to check if we are going in the right direction or not.

For testing we should pull a database of EDID files from one or multiple sources (specially prepared ones and from actual displays) instead of hardcoding some EDIDs in C files.

Sure, we can reuse some existing EDID DB as discussed in other threads.

One thing I don't like is that this assumes the only EDID extension is CTA. The other important one is DisplayID but there might be more.

Agree, that is one important item in todo list, to expand the coverage for more extensions, once we have the basic stuff in place.

The error handling in libedid_process_edid_info is not great. Maybe an opaque context struct might be useful, maybe just a parse error struct as return parameter.

pq also had some ideas regarding forwards and backwards compatibility.

Noted, will probably document this somewhere in a todo.txt

The high level API might require information about what mode the display is in including active AVI InfoFrames.

I am not sure if I got this right. The AVI IF would be only available in the display driver, as its prepared in there. In many hardwares (like Intel) the display controller prepares the AVI IF to be send over the protocol. I am not sure if we can get AVI IF in userspace.
Maybe a struct dl_display_state that can be set up once and then passed to the high level API calls along with the EDID info.

This is a great start!

Thank you.

@swick
Copy link

swick commented Mar 23, 2022

Sure, we can reuse some existing EDID DB as discussed in other threads.

Ah, missed that. Perfect.

Agree, that is one important item in todo list, to expand the coverage for more extensions, once we have the basic stuff in place.

My point wasn't so much about actually parsing other extensions than the layout of struct edid_info but I guess for other extensions we support just adding them there would work fine so nvm.

I am not sure if I got this right. The AVI IF would be only available in the display driver, as its prepared in there. In many hardwares (like Intel) the display controller prepares the AVI IF to be send over the protocol. I am not sure if we can get AVI IF in userspace.

This doesn't have to be the exact AVI IF layout but for example if you want to know the transfer characteristics you get different answers depending on if the HDR IF is sent and what data it contains.

e: This display state is not something the library would apply. It would only be used to return the correct answer for a specific question.

@swick
Copy link

swick commented Mar 24, 2022

Another thing to consider is that DisplayID technically can exist not as as E-EDID extension but on its own. The struct edid_info probably has to handle that somehow and we definitely need another entrypoint for a standalone DisplayID blob.

@ppaalanen
Copy link
Author

I see that Simon created the new repository at https://gitlab.freedesktop.org/emersion/libdisplay-info, so we should discuss everything in issues and merge requests there. You wouldn't push a single line of code in there without someone else having reviewed it.

Collecting all discussions into one project/repository is good.

It would also be good to start simple, adding API bit by bit, and not 28 accessor functions in one go. That way we can adjust the API style without you needing rewrite dozens of functions several times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants