Re: [PATCH 1/3] edid-decode: Introduce libedid-decode wrapper

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4584 bytes --]

On Fri,  4 Mar 2022 13:49:59 +0100
Shashank Sharma <contactshashanksharma@gmail.com> wrote:

> From: Shashank Sharma <shashank.sharma@amd.com>
> 
> 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@gmail.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]