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. > + > 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? > 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. Thanks, pq > + } > + > + return NULL; > +} > > static unsigned char crc_calc(const unsigned char *b) > {
Attachment:
pgpzXLT6nLwDC.pgp
Description: OpenPGP digital signature