Chrome vs spec: encoder.encodeQueueSize

[chcunningham]

Chrome's implementation of encoder.encodeQueueSize (and decoder counterpart) doesn't match the spec. The difference is fairly minor, but I think authors will prefer Chrome's behavior. It ends up being a bit of yak shave...

Background

The spec/impl mismatch stems from Chrome's impl of the codec processing model.

The spec defines a control message queue with messages enqueued on the control thread and dequeued on the codec thread. In Chrome's implementation both enqueuing and dequeuing actually happen on the control thread. We use a separate mechanism ("post task") to communicate with the codec thread (where encoding actually happens). This is effectively also a queue, but it's distinct from the control message queue. For the spec, combining control-signal-queuing and thread-message-passing seemed like a nice simplification with no difference in observable behavior... almost.

The issue arises with when to decrement encodeQueueSize.

Per spec, it takes at least one turn of the event loop to learn that your encode() has been processed, even in cases where the encoder wasn't saturated and could accommodate the request right away. This is a consequence of the dequeue happening on the codec thread and posting a task to update the counter.

In Chrome's impl, authors may instead observe cases where encode() was immediately processed without waiting a turn of the event loop. In such cases the control message is dequeued before encode() even returns and we don't increment the encodeQueueSize counter.

Proposing a spec change

The purpose of the encodeQueueSize attribute is to give authors a way of monitoring back pressure from the underlying codec. Incrementing for every encode() and decrementing sometime later adds unpleasant indirection.

Having the spec match Chrome requires updating the "Codec Processing Model" to process the queue entirely on the main thread. This is a big editorial change, but the only author-observable difference would be for encodeQueueSize. Finding this bug has made me nervous that the current Codec Processing Model elides too much. Making it match the impl more closely seems prudent.

Please see a preview of the spec change in #493.

[chcunningham]

Summarizing editors call:

@padenot was a bit concerned that moving the increment/decrement of queue size to main thread could result us modifying the count while scripts are running. Will ponder it some more.

On the call I mentioned that I think we avoid the issue, but maybe the spec could make this more explicit. My intent is that there should be no scenario where authors can observe the counter change while scripts are running, except for cases that are directly tied to authors invoking encode().

The 2 scenarios are:

1. encode() is immediately ingested. In this case counter will be 0 both before and after the call.
2. encode() ingestion is delayed (e.g. encoder saturated). When encode() returns authors may observe that the counter has incremented by 1. The counter should later decrement (when encoder completes enough work to ingest this new encode request), and that should not happen while scripts are running. This last part could be more clear... let me massage the PR a bit.

We also talked a bit about an encoder that was always ready to accept more inputs. For example, libOpus audio encoder is probably in this camp. In this case you're just always in scenario 1, so seems ok. In fact, I think it's a point in favor of moving the queue processing to the main thread: it affords authors the ability to observe this sort of encoder behavior should the desire to push larger batches of work.

[chcunningham]

@padenot

The counter should later decrement (when encoder completes enough work to ingest this new encode request), and that should not happen while scripts are running. This last part could be more clear... let me massage the PR a bit.

The latest commit makes this explicit.

[chcunningham]

FYI @sbalko

[padenot]

We also talked a bit about an encoder that was always ready to accept more inputs. For example, libOpus audio encoder is probably in this camp. In this case you're just always in scenario 1, so seems ok. In fact, I think it's a point in favor of moving the queue processing to the main thread: it affords authors the ability to observe this sort of encoder behavior should the desire to push larger batches of work.

generally, if there is some kind of buffering at the input of the encoder that makes it always capable of ingesting packets, I think I used libopus, and specifically its integration in Firefox as an illustration. libopus API is sychronous but we have an async layer on top, meaning we can push a lot of packets.

Generally we won't have saturated decoders, ever, it will just buffer at the input, in a queue. istr you mentionned an implementation-imposed limit, we could do that if we wanted to limit memory usage.

[chcunningham]

@sandersdan, can you drive firming up alignment w/ @padenot while I'm OOO? I can update the PR when I return.

@padenot was a bit concerned that moving the increment/decrement of queue size to main thread could result us modifying the count while scripts are running. Will ponder it some more.

Did the latest commit address this concern? Are we aligned enough that I could start updating other methods in the PR?

Generally we won't have saturated decoders, ever, it will just buffer at the input, in a queue.

I think we're probably talking about the same thing. My PR describes saturation as a property of the "codec implementation", but in reality it may be a limit imposed by the UAs video stack. I think this distinction probably doesn't make a difference to readers, but happy to massage text if you like.

istr you mentionned an implementation-imposed limit, we could do that if we wanted to limit memory usage.

In our case, at least on the decoder side, I think the limit is an artifact of our streaming pipeline design. Our internal codec interface has a GetMaxDecodeRequests() API which semantically means "you can send me N decodes() before blocking on my signal that I'm ready for another one". In practice we use N=4 for hw accelerated decoders and N=1 otherwise - so definitely more a UA specific limit vs a codec implementation limit. The higher limit for hw accelerated decoders was motivated by some hw decoders having longer pipeline depth and also helps mitigate added latency from IPC.

[aboba]

A few questions:

1. In some descriptive text (though not in the PR), there is a reference to the "main thread". Is it true that the "control thread" is the thread on which the decoder or encoder was constructed? For example, where an encoder or decoder was constructed in a worker, the control thread would be in the worker, not the main thread, correct?

2. How can an application discover the maximum encodeQueueSize/decodeQueueSize, after which encode/decode operations would block?

3. Can encode/decode methods block even when the maximum encodeQueueSize/decodeQueueSize has not been reached? For example, there are reports of encode in a worker taking 30 ms.

[sandersdan]

Is it true that the "control thread" is the thread on which the decoder or encoder was constructed?

The main thread is the JS thread, either a document or worker context. The control thread is where the codec operates, asynchronously from whatever JS is doing.

How can an application discover the maximum encodeQueueSize/decodeQueueSize, after which encode/decode operations would block?

There should never be blocking behavior. I did expect that we would eventually throw QuotaExceeded but no specific limits were ever picked. I believe the queues are currently unbounded.

Can encode/decode methods block even when the maximum encodeQueueSize/decodeQueueSize has not been reached?

I would consider that a quality-of-implementation question. Chrome's implementation of encode() does in some cases result in synchronous readback of hardware frames, which can be high-latency; we have plans to improve that behavior.

[chcunningham]

EC: Latest commit addresses concerns. OK to replicate AudioDecoder changes throughout spec.

[chcunningham]

EC: Latest commit addresses concerns. OK to replicate AudioDecoder changes throughout spec.

Done. PR ready for review.

[chcunningham]

Fixed by #493