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

[Ilia Mirkin <imirkin@alum.mit.edu>]

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