[PATCH 0/2] edid-decode: add emscripten support

[PATCH 0/2] edid-decode: add emscripten support

[Ilia Mirkin]

Demo available here:

https://people.freedesktop.org/~imirkin/edid-decode/

This is enough to generate the edid-decode.{js,wasm} files, and the html
was hand-modified from the demo one to remove a bunch of unnecessary
bits and provide the required integration. Happy to move it into the
project too if desired.

I'm a complete emscripten newbie, so I'm not 100% sure I've set this up
correctly, as alluded in patch 2's comments. But it does seem to work.
Given that the patches aren't crazy, I think it's reasonable to carry
them inside of the project.

Ilia Mirkin (2):
  edid-decode: add function to enable usage as a library
  edid-decode: add build rule for emscripten output

 Makefile        |  5 +++++
 edid-decode.cpp | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

-- 
2.24.1

[PATCH 1/2] edid-decode: add function to enable usage as a library

[Ilia Mirkin]

This is helpful for an emscripten setup, as there's no great way to
reinitialize the object from scratch.

Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
---
 edid-decode.cpp | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/edid-decode.cpp b/edid-decode.cpp
index ef50da2..fd17bc6 100644
--- a/edid-decode.cpp
+++ b/edid-decode.cpp
@@ -1005,3 +1005,21 @@ int main(int argc, char **argv)
 		return edid_from_file(argv[optind], argv[optind + 1], out_fmt);
 	return ret ? ret : state.parse_edid();
 }
+
+#ifdef __EMSCRIPTEN__
+/*
+ * The surrounding JavaScript implementation will call this function
+ * each time it wants to decode an EDID. So this should reset all the
+ * state and start over.
+ */
+extern "C" int parse_edid(const char *input) {
+	for (unsigned i = 0; i < EDID_MAX_BLOCKS + 1; i++) {
+		s_msgs[i][0].clear();
+		s_msgs[i][1].clear();
+	}
+	options[OptCheck] = 1;
+	state = edid_state();
+	int ret = edid_from_file(input, NULL, OUT_FMT_DEFAULT);
+	return ret ? ret : state.parse_edid();
+}
+#endif
-- 
2.24.1

[PATCH 2/2] edid-decode: add build rule for emscripten output

[Ilia Mirkin]

All I can really say is that this works in my current setup. This
current setup was not the first setup I tried, with lots of random
flailing around emscripten and binaryen versions to be compatible with
my LLVM version. Perhaps later emscripten needs slightly different flags
to export functions, who knows.

Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index 6db6386..49ad39b 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,8 @@
 bindir ?= /usr/bin
 mandir ?= /usr/share/man
 
+EMXX ?= em++
+
 SOURCES = edid-decode.cpp parse-base-block.cpp parse-cta-block.cpp \
 	  parse-displayid-block.cpp parse-ls-ext-block.cpp \
 	  parse-di-ext-block.cpp parse-vtb-ext-block.cpp
@@ -13,6 +15,9 @@ sha = -DSHA=$(shell if [ -d .git ]; then git rev-parse HEAD ; else printf '"not
 edid-decode: $(SOURCES) edid-decode.h Makefile
 	$(CXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) -g $(sha) -o $@ $(SOURCES) -lm
 
+edid-decode.js: $(SOURCES) edid-decode.h Makefile
+	$(EMXX) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) $(WARN_FLAGS) $(sha) -s EXPORTED_FUNCTIONS='["_parse_edid"]' -s EXTRA_EXPORTED_RUNTIME_METHODS='["ccall", "cwrap"]' -o $@ $(SOURCES) -lm
+
 clean:
 	rm -f edid-decode
 
-- 
2.24.1

Re: [PATCH 1/2] edid-decode: add function to enable usage as a library

[Hans Verkuil]

Hi Ilia,

I pushed my patch to initialize edid_state in the constructor to the
edid-decode git repo.

I've also split off the 'write to a file' part from edid_from_file into
a separate edid_to_file function, so you'll have to rebase your patch.

Two comments below:

On 2/9/20 6:49 PM, Ilia Mirkin wrote:
> This is helpful for an emscripten setup, as there's no great way to
> reinitialize the object from scratch.
> 
> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> ---
>  edid-decode.cpp | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/edid-decode.cpp b/edid-decode.cpp
> index ef50da2..fd17bc6 100644
> --- a/edid-decode.cpp
> +++ b/edid-decode.cpp
> @@ -1005,3 +1005,21 @@ int main(int argc, char **argv)
>  		return edid_from_file(argv[optind], argv[optind + 1], out_fmt);
>  	return ret ? ret : state.parse_edid();
>  }
> +
> +#ifdef __EMSCRIPTEN__
> +/*
> + * The surrounding JavaScript implementation will call this function
> + * each time it wants to decode an EDID. So this should reset all the
> + * state and start over.
> + */
> +extern "C" int parse_edid(const char *input) {

'{' should start on the next line. Let's keep the coding style consistent.

> +	for (unsigned i = 0; i < EDID_MAX_BLOCKS + 1; i++) {
> +		s_msgs[i][0].clear();
> +		s_msgs[i][1].clear();
> +	}

I'm pretty sure that this for loop is not necessary. Can you drop this
for loop and see if it works?

> +	options[OptCheck] = 1;
> +	state = edid_state();
> +	int ret = edid_from_file(input, NULL, OUT_FMT_DEFAULT);
> +	return ret ? ret : state.parse_edid();
> +}
> +#endif
> 

Regards,

	Hans

Re: [PATCH 1/2] edid-decode: add function to enable usage as a library

[Ilia Mirkin]

On Mon, Feb 10, 2020 at 6:23 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Hi Ilia,
>
> I pushed my patch to initialize edid_state in the constructor to the
> edid-decode git repo.

Hmm... I thought it should already work before that. Things should get
zero-initialized in C++ constructors. There are some subtleties with
PODs though, but I don't think they apply here. But it's been a while
since I've looked at those C++ details, and your update constructor
definitely doesn't hurt.

>
> I've also split off the 'write to a file' part from edid_from_file into
> a separate edid_to_file function, so you'll have to rebase your patch.

Will do.

>
> Two comments below:
>
> On 2/9/20 6:49 PM, Ilia Mirkin wrote:
> > This is helpful for an emscripten setup, as there's no great way to
> > reinitialize the object from scratch.
> >
> > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> > ---
> >  edid-decode.cpp | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/edid-decode.cpp b/edid-decode.cpp
> > index ef50da2..fd17bc6 100644
> > --- a/edid-decode.cpp
> > +++ b/edid-decode.cpp
> > @@ -1005,3 +1005,21 @@ int main(int argc, char **argv)
> >               return edid_from_file(argv[optind], argv[optind + 1], out_fmt);
> >       return ret ? ret : state.parse_edid();
> >  }
> > +
> > +#ifdef __EMSCRIPTEN__
> > +/*
> > + * The surrounding JavaScript implementation will call this function
> > + * each time it wants to decode an EDID. So this should reset all the
> > + * state and start over.
> > + */
> > +extern "C" int parse_edid(const char *input) {
>
> '{' should start on the next line. Let's keep the coding style consistent.

Right, my bad.

>
> > +     for (unsigned i = 0; i < EDID_MAX_BLOCKS + 1; i++) {
> > +             s_msgs[i][0].clear();
> > +             s_msgs[i][1].clear();
> > +     }
>
> I'm pretty sure that this for loop is not necessary. Can you drop this
> for loop and see if it works?

The current code has:

static void show_msgs(bool is_warn)
{
  printf("\n%s:\n\n", is_warn ? "Warnings" : "Failures");
  for (unsigned i = 0; i < state.num_blocks; i++) {
    if (s_msgs[i][is_warn].empty())
      continue;
    print-the-error

What would prevent an error from a previous run to appear without an
explicit clearing of s_msgs?

>
> > +     options[OptCheck] = 1;
> > +     state = edid_state();
> > +     int ret = edid_from_file(input, NULL, OUT_FMT_DEFAULT);
> > +     return ret ? ret : state.parse_edid();
> > +}
> > +#endif
> >
>
> Regards,
>
>         Hans

Re: [PATCH 1/2] edid-decode: add function to enable usage as a library

[Ilia Mirkin]

On Mon, Feb 10, 2020 at 9:58 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Mon, Feb 10, 2020 at 6:23 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > > +     for (unsigned i = 0; i < EDID_MAX_BLOCKS + 1; i++) {
> > > +             s_msgs[i][0].clear();
> > > +             s_msgs[i][1].clear();
> > > +     }
> >
> > I'm pretty sure that this for loop is not necessary. Can you drop this
> > for loop and see if it works?
>
> The current code has:
>
> static void show_msgs(bool is_warn)
> {
>   printf("\n%s:\n\n", is_warn ? "Warnings" : "Failures");
>   for (unsigned i = 0; i < state.num_blocks; i++) {
>     if (s_msgs[i][is_warn].empty())
>       continue;
>     print-the-error
>
> What would prevent an error from a previous run to appear without an
> explicit clearing of s_msgs?

Hi Hans,

Do you agree with my assessment above?

  -ilia

Re: [PATCH 1/2] edid-decode: add function to enable usage as a library

[Hans Verkuil]

On 2/13/20 1:48 AM, Ilia Mirkin wrote:
> On Mon, Feb 10, 2020 at 9:58 AM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Mon, Feb 10, 2020 at 6:23 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>> +     for (unsigned i = 0; i < EDID_MAX_BLOCKS + 1; i++) {
>>>> +             s_msgs[i][0].clear();
>>>> +             s_msgs[i][1].clear();
>>>> +     }
>>>
>>> I'm pretty sure that this for loop is not necessary. Can you drop this
>>> for loop and see if it works?
>>
>> The current code has:
>>
>> static void show_msgs(bool is_warn)
>> {
>>   printf("\n%s:\n\n", is_warn ? "Warnings" : "Failures");
>>   for (unsigned i = 0; i < state.num_blocks; i++) {
>>     if (s_msgs[i][is_warn].empty())
>>       continue;
>>     print-the-error
>>
>> What would prevent an error from a previous run to appear without an
>> explicit clearing of s_msgs?
> 
> Hi Hans,
> 
> Do you agree with my assessment above?
> 
>   -ilia
> 

You are right, sorry. I should have checked the actual code instead of
relying on my obviously faulty memory.

Regards,

	Hans