Hello Pekka,
On 07.03.22 16:54, Pekka Paalanen wrote:
On Fri, 4 Mar 2022 13:49:59 +0100
Shashank Sharma <contactshashanksharma@xxxxxxxxx> wrote:
From: Shashank Sharma <shashank.sharma@xxxxxxx>
This patch does some small changes to make the core logic of
edid-decode tool available to a shared library wrapper. With
these changes, the EDID's 'state' variable will be avialble
to another process via some library API calls.
Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Signed-off-by: Shashank Sharma <contactshashanksharma@xxxxxxxxx>
Hi Shashank,
thank you very much for working on this!
---
Makefile | 22 +++++++++++++++++++++-
edid-decode.cpp | 15 ++++++++++++++-
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 1843700..ebf3370 100644
--- a/Makefile
+++ b/Makefile
@@ -1,14 +1,20 @@
ifeq ($(OS),Windows_NT)
bindir ?= /usr/bin
mandir ?= /usr/share/man
+ libdir ?= /usr/lib
+ includedir ?= /usr/include
else
UNAME_S := $(shell uname -s)
ifeq ($(UNAME_S),Darwin)
bindir ?= /usr/local/sbin
mandir ?= /usr/local/share/man
+ libdir ?= /usr/local/lib
+ includedir ?= /usr/include
else
bindir ?= /usr/bin
mandir ?= /usr/share/man
+ libdir ?= /usr/lib
+ includedir ?= /usr/include
endif
endif
@@ -19,6 +25,11 @@ SOURCES = edid-decode.cpp parse-base-block.cpp parse-cta-block.cpp \
parse-di-ext-block.cpp parse-vtb-ext-block.cpp calc-gtf-cvt.cpp
WARN_FLAGS = -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wimplicit-fallthrough
+LIB_NAME = libedid-decode.so
+LIB_FLAGS = -fPIC
+LIBLINK_FLAGS = -shared
+LIB_SOURCES = libedid-decode-api.cpp
libedid-decode-api.cpp does not exist yet in this patch.
Yes, the API is introduced in patch 2, Noted.
+
all: edid-decode
sha = -DSHA=$(shell if test -d .git ; then git rev-parse --short=12 HEAD ; fi)
@@ -30,11 +41,20 @@ edid-decode: $(SOURCES) edid-decode.h oui.h Makefile
edid-decode.js: $(SOURCES) edid-decode.h oui.h Makefile
$(EMXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) $(sha) $(date) -s EXPORTED_FUNCTIONS='["_parse_edid"]' -s EXTRA_EXPORTED_RUNTIME_METHODS='["ccall", "cwrap"]' -o $@ $(SOURCES) -lm
+libedid-decode: $(SOURCES) edid-decode.h oui.h Makefile
+ $(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(LIB_FLAGS) $(sha) $(date) $(LIBLINK_FLAGS) -o $(LIB_NAME) $(LIB_SOURCES) $(SOURCES) -lm
+
clean:
- rm -f edid-decode
+ rm -f edid-decode libedid-decode.so
install:
mkdir -p $(DESTDIR)$(bindir)
install -m 0755 edid-decode $(DESTDIR)$(bindir)
mkdir -p $(DESTDIR)$(mandir)/man1
install -m 0644 edid-decode.1 $(DESTDIR)$(mandir)/man1
+
+install-lib:
+ mkdir -p $(DESTDIR)$(libdir)
+ mkdir -p $(DESTDIR)$(includedir)
+ install -m 0755 libedid-decode.so $(DESTDIR)$(libdir)
+ install -m 0644 libedid-decode-api.h $(DESTDIR)$(includedir)
libedid-decode-api.h does not exist yet in this patch.
I find it a little odd to have these targets here without the actual
files. Maybe the first patch could already have a library building but
expose just parse and destroy functions without any getters yet?
Yes, this is more like a formatting error. I need to move these files to
patch 2, where they are introduced.
diff --git a/edid-decode.cpp b/edid-decode.cpp
index 4a90aba..babff4a 100644
--- a/edid-decode.cpp
+++ b/edid-decode.cpp
@@ -21,7 +21,7 @@
#define STR(x) #x
#define STRING(x) STR(x)
-static edid_state state;
+edid_state state;
static unsigned char edid[EDID_PAGE_SIZE * EDID_MAX_BLOCKS];
static bool odd_hex_digits;
@@ -1012,6 +1012,19 @@ static bool extract_edid(int fd, FILE *error)
state.edid_size = edid_data.size();
return true;
}
+struct edid_state *extract_edid_state(int fd, FILE *error)
+{
+ bool ret;
+
+ ret = extract_edid(fd, error);
+ if (ret) {
+ /* update the number of blocks */
+ state.num_blocks = state.edid_size / EDID_PAGE_SIZE;
+ return &state;
A library should not give out pointers to global mutable data. That
would break having multiple EDIDs loaded at the same time.
I would expect to be able to keep and cache 'struct edid_state'
instances created by this library until I explicitly destroy them.
I would not expect parsing a new EDID to overwrite the previously
returned object. IOW, I would expect to own the object created by the
library.
Till now, I was under the impression of a design where a compositor
parses the EDID, and saves all the information in its state immediately,
so that when the second EDID is loaded, it can override first one. But
based on your inputs I myself feel that its a bit rigid. Now I am
thinking about extending it to something which remains until the process
lifetime. How does this look to you:
- The compositor passes the EDID file node to library.
- The library parses the EDID, creates a state variable and caches it,
and gives back a handle(unique) to the compositor.
/* in compositor's display/connector init part */
connector.handle = libedid_parse_edid(EDID_NODE);
- While calling the subsequent APIs, compositor passes the handle with
the API, like
/* Somewhere later in the same compositor */
ret = libedid_is_ycbcr420_supported(connector.handle);
if (ret) {
/* Prepare a YCBCR420 modeset */
}
and so on .....
This should address your concerns as well.
- Shashank
Thanks,
pq
+ }
+
+ return NULL;
+}
static unsigned char crc_calc(const unsigned char *b)
{
