| Message ID | 20200209174937.22844-2-imirkin@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | edid-decode: add emscripten support | expand |
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
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
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
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
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
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(+)