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
