Skip to content

Revise processing model, fix queue size attributes #493

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

Merged
merged 12 commits into from
Jul 28, 2022

Conversation

chcunningham
Copy link
Collaborator

@chcunningham chcunningham commented May 16, 2022

A few changes

  • Updates process model to process control message queue entirely on the main thread. This aligns spec with Chrome, fixing Chrome vs spec: encoder.encodeQueueSize #494.
  • Moving ^that processing to the main thread means we need to be more explicit about when the loop is blocked/exited. For example, an implementation may block processing a decode() call when it finds the decoder is saturated by active decodes. Previously we elided description of this blocking behavior because the blocked loop lived on another thread, which was unobservable (beyond the queue size attributes). But with message processing happening entirely on the main (control) thread, we're motivated to be more explicit (we can't block scripts).
  • Named "codec" and "control" threads are removed to be more idiomatic with other W3C specs. WebAudio has named threads, but I've learned that isn't typical. This isn't strictly necessary, but it seems like a smart time to make the change (I'm re-writing all references to these threads anyway).

Preview | Diff

@chcunningham chcunningham changed the title Revise processing model, begin transitioning AudioDecoder Process control messages on main thread, fix queue size attributes May 16, 2022
@chcunningham chcunningham changed the title Process control messages on main thread, fix queue size attributes Revise processing model, fix queue size attributes May 16, 2022
@chcunningham
Copy link
Collaborator Author

@padenot @aboba - review guide:

This first commit revises the processing model and updates AudioDecoder.configure() and AudioDecoder.decode() to use the new way. Will go further after getting some initial feedback.

@chcunningham
Copy link
Collaborator Author

@sandersdan FYI

@chcunningham chcunningham requested review from aboba and padenot July 2, 2022 07:56
@chcunningham
Copy link
Collaborator Author

@aboba @padenot, PTAL

@chcunningham
Copy link
Collaborator Author

@padenot has gone OOO for a bit, so planning to TBR for his review (he's already blessed the first round of change for AudioDecoder).

@dalecurtis, could you take a look at the ImageDecoder changes here. Should be no change at all to Chrome's behavior - just editorial refactoring to align spec:chrome. Happy to go through it on VC if you'd like.

Copy link
Contributor

@dalecurtis dalecurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to make things more complicated than necessary for ImageDecoder, but it's probably better to be consistent throughout the document on the processing model.

instance. See [=[[control message queue]]=].

: <dfn attribute for=ImageDecoder>[[message queue blocked]]</dfn>
:: A boolean indicating when processing the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there are no operations which can block the work queue for ImageDecoder. The only things close are reset() and track changes, both of which will abort the queue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor has an async step to configure the decoder, similar to how configure() works for the other interfaces. This gets queued as a control message and blocks any subsequent decode() until configure resolves. It may be that our impl is actually simpler than this, and I don't know that the blockage is really observable anyway, but it seemed nice to be consistent w/ the other configure() control messages in this way.

@@ -4870,7 +5073,7 @@
6. Return `true`.

A <dfn>valid image MIME type</dfn> is a string that is a [=valid MIME type
string=] and for which the `type`, per Section 3.1.1.1 of [[RFC7231]], is
string=] and for which the `type`, per Section 3.1.1.1 of [[RFC9110]], is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a typo before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed let me know that the original RFC was obsoleted by this new one.

Copy link
Collaborator Author

@chcunningham chcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dalecurtis.

I definitely agree w/ your sentiment that this is more complicated than I'd make it were ImageDecoder a standalone thing.

instance. See [=[[control message queue]]=].

: <dfn attribute for=ImageDecoder>[[message queue blocked]]</dfn>
:: A boolean indicating when processing the
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor has an async step to configure the decoder, similar to how configure() works for the other interfaces. This gets queued as a control message and blocks any subsequent decode() until configure resolves. It may be that our impl is actually simpler than this, and I don't know that the blockage is really observable anyway, but it seemed nice to be consistent w/ the other configure() control messages in this way.

@@ -4870,7 +5073,7 @@
6. Return `true`.

A <dfn>valid image MIME type</dfn> is a string that is a [=valid MIME type
string=] and for which the `type`, per Section 3.1.1.1 of [[RFC7231]], is
string=] and for which the `type`, per Section 3.1.1.1 of [[RFC9110]], is
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed let me know that the original RFC was obsoleted by this new one.

@chcunningham
Copy link
Collaborator Author

Thanks everyone. @padenot, I'll go ahead and merge as TBR while you're OOO. Late feedback welcome.

@chcunningham chcunningham merged commit e24686d into main Jul 28, 2022
@chcunningham chcunningham deleted the revise_processing_model_fix_queue_size branch July 28, 2022 20:55
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 28, 2022
SHA: e24686d
Reason: push, by @chcunningham

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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