Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compiling and add sample decoder #84

Merged
merged 9 commits into from
Jun 15, 2021
Merged

Conversation

nasirhemed
Copy link
Collaborator

Issues with Compiling

I was running into issues when trying to build on my local and realized that there were some issues.

typescript was complaining about type issues in node_modules/**/.d.ts files. To fix that, I upgraded to typescript 4.2.4

There were some this.document.<attr> that weren't referenced as (this.document as any).<attr> so typescript complained that document does not have 'attr'. I added as any to fix that issue.

Finally, tsconfig had some redundant config values so I just removed them.

Added Sample Decoder

I also added a sample_decoder to include inspect.js and inspect.wasm files

@negge negge requested a review from tdaede May 25, 2021 20:21
@tdaede
Copy link
Collaborator

tdaede commented May 25, 2021

This looks OK.

The sample decoders are technically build artifacts, I'm not necessarily opposed to including them in-tree but it would be good for them to be reproducible. At minimum I'd like a script and the exact libaom versions you used to generate them.

src/components/Analyzer.tsx Outdated Show resolved Hide resolved
@nasirhemed
Copy link
Collaborator Author

@tdaede
Copy link
Collaborator

tdaede commented Jun 13, 2021

OK. I'm still not super hot on the idea of the .wasm/.js build artifacts being checked into git - I think they are nice thing to have but I don't really like committing build artifacts.

If you split those commits into a separate PR so we can discuss it further, we can land the other changes in this PR.

@nasirhemed
Copy link
Collaborator Author

Okay I'll remove the artifacts for this PR. I still think we need to have a way for the user to have access to inspect.js/inspect.wasm as there aren't instructions on how to get them and the emscripten build for aom is currently a bit messy.

I'll create a draft PR so that we can discuss this further

@nasirhemed nasirhemed requested a review from tdaede June 14, 2021 19:58
@tdaede tdaede merged commit dcb07fc into xiph:master Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants