Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1836953002:
Motown: Delay audio track completion of Prime requests until a packet arrives. (Closed)
DescriptionMotown: Delay audio track completion of Prime requests until a packet arrives.
The audio track currently does nothing about priming (which is fine) and
calls the callback immediately when a Prime request arrives (which is not
fine). Ideally, it would wait to call the callback until its queue had reached
its low-water mark. This CL gets partway there by waiting until the first
packet arrives. This doesn't necessarily address potential underflow issues,
but it does make sure we've seen at least one packet so we know how to set
the rate control transform.
R=kulakowski@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/fb1c0c9b02d44e74cffb6d6881f25b4f9eb50ec9
Patch Set 1 #
Total comments: 7
Messages
Total messages: 14 (3 generated)
Description was changed from ========== Motown: Delay audio track completion of Prime requests until a packet arrives. The audio track currently does nothing about priming (which is fine) and calls the callback immediately when a Prime request arrives (which is not fine). Ideally, it would wait to call the callback until its queue had reached its low-water mark. This CL gets partway there by waiting until the first packet arrives. This doesn't necessarily address potential underflow issues, but it does make sure we've seen at least one packet so we know how to set the rate control transform. ========== to ========== Motown: Delay audio track completion of Prime requests until a packet arrives. The audio track currently does nothing about priming (which is fine) and calls the callback immediately when a Prime request arrives (which is not fine). Ideally, it would wait to call the callback until its queue had reached its low-water mark. This CL gets partway there by waiting until the first packet arrives. This doesn't necessarily address potential underflow issues, but it does make sure we've seen at least one packet so we know how to set the rate control transform. ==========
dalesat@chromium.org changed reviewers: + johngro@google.com, kulakowski@chromium.org
Please take a look. Thanks!
https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. For my edification: What is priming? What is a "low water mark"?
https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. On 2016/03/28 20:46:12, kulakowski wrote: > For my edification: What is priming? What is a "low water mark"? Priming is getting some initial packets of content in place so that renderers don't starve when we 'play'. We want some content queued up so the renderer has something to chew on right away. The low water mark is the least amount of content we can have queued at the renderer and still be confident that it won't starve due to whatever delays happen elsewhere in the graph. Right now, we're not experiencing any starvation when we have one packet ready...but that's presumably dependent on how much data is in the packet and some other stuff. The primary concern at the moment is that we want to see at least one packet so we know what timestamp it has. When we start at the beginning of the file, the timestamp will typically be 0. When we seek, the timestamp will be something quite different. We need to know the timestamp, because we do rate control by saying 'the packet with timestamp X needs to be presented at real time Y'.
https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. On 2016/03/28 22:19:56, dalesat wrote: > On 2016/03/28 20:46:12, kulakowski wrote: > > For my edification: What is priming? What is a "low water mark"? > > Priming is getting some initial packets of content in place so that renderers > don't starve when we 'play'. We want some content queued up so the renderer has > something to chew on right away. > > The low water mark is the least amount of content we can have queued at the > renderer and still be confident that it won't starve due to whatever delays > happen elsewhere in the graph. Right now, we're not experiencing any starvation > when we have one packet ready...but that's presumably dependent on how much data > is in the packet and some other stuff. > > The primary concern at the moment is that we want to see at least one packet so > we know what timestamp it has. When we start at the beginning of the file, the > timestamp will typically be 0. When we seek, the timestamp will be something > quite different. We need to know the timestamp, because we do rate control by > saying 'the packet with timestamp X needs to be presented at real time Y'. Gotcha. Couple more questions. How is seeking different from starting? You said "typically 0", which I guess isn't a priori clear to me that it would always be the case. What are the atypical cases? How is an AudioPipe supposed to be able to calculate this amount of content, for low water marking?
PTAL https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. On 2016/03/30 20:44:55, kulakowski wrote: > On 2016/03/28 22:19:56, dalesat wrote: > > On 2016/03/28 20:46:12, kulakowski wrote: > > > For my edification: What is priming? What is a "low water mark"? > > > > Priming is getting some initial packets of content in place so that renderers > > don't starve when we 'play'. We want some content queued up so the renderer > has > > something to chew on right away. > > > > The low water mark is the least amount of content we can have queued at the > > renderer and still be confident that it won't starve due to whatever delays > > happen elsewhere in the graph. Right now, we're not experiencing any > starvation > > when we have one packet ready...but that's presumably dependent on how much > data > > is in the packet and some other stuff. > > > > The primary concern at the moment is that we want to see at least one packet > so > > we know what timestamp it has. When we start at the beginning of the file, the > > timestamp will typically be 0. When we seek, the timestamp will be something > > quite different. We need to know the timestamp, because we do rate control by > > saying 'the packet with timestamp X needs to be presented at real time Y'. > > Gotcha. Couple more questions. > > How is seeking different from starting? You said "typically 0", which I guess > isn't a priori clear to me that it would always be the case. What are the > atypical cases? > > How is an AudioPipe supposed to be able to calculate this amount of content, for > low water marking? Content is typically authored such that the first packets in each stream are timestamped roughly 0. Some content could just be weird and start at some other value. If we're hooking up to a live feed, the initial timestamp could be anything. When we seek, we pause playback, flush all the packets in the pipeline, tell the demux (source of packets) to seek to a new position, and then prime. In that case, the first packet to arrive at the audio track will have a timestamp that roughly corresponds to our seek position (which can be zero). Calculating the low water mark is mostly a matter of picking a number (probably a time interval) that works (prevents underflow) consistently. A lot of factors can affect how low this value can be, including stuff like scheduling latency. In the end, we will probably come up with a hard-coded time interval that always seems to work based on our testing. We might also let an application set this, so it can balance risk of underflow vs latency.
https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. > Calculating the low water mark is mostly a matter of picking a number (probably > a time interval) that works (prevents underflow) consistently. A lot of factors > can affect how low this value can be, including stuff like scheduling latency. > In the end, we will probably come up with a hard-coded time interval that always > seems to work based on our testing. We might also let an application set this, > so it can balance risk of underflow vs latency. It really does not seem to me that this is a single policy audio_pipe needs to create. A single hardcoded interval doesn't seem like it will be correct for everyone.
https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. On 2016/03/31 21:32:12, kulakowski wrote: > > Calculating the low water mark is mostly a matter of picking a number > (probably > > a time interval) that works (prevents underflow) consistently. A lot of > factors > > can affect how low this value can be, including stuff like scheduling latency. > > In the end, we will probably come up with a hard-coded time interval that > always > > seems to work based on our testing. We might also let an application set this, > > so it can balance risk of underflow vs latency. > > It really does not seem to me that this is a single policy audio_pipe needs to > create. A single hardcoded interval doesn't seem like it will be correct for > everyone. Sure. Is this a concern for this CL?
https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... File services/media/audio/audio_pipe.cc (right): https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until low water mark is reached. On 2016/03/31 21:45:14, dalesat wrote: > On 2016/03/31 21:32:12, kulakowski wrote: > > > Calculating the low water mark is mostly a matter of picking a number > > (probably > > > a time interval) that works (prevents underflow) consistently. A lot of > > factors > > > can affect how low this value can be, including stuff like scheduling > latency. > > > In the end, we will probably come up with a hard-coded time interval that > > always > > > seems to work based on our testing. We might also let an application set > this, > > > so it can balance risk of underflow vs latency. > > > > It really does not seem to me that this is a single policy audio_pipe needs to > > create. A single hardcoded interval doesn't seem like it will be correct for > > everyone. > > Sure. Is this a concern for this CL? Well if you have a TODO about it in the implementation of it, it seems to be to me.
On 2016/03/31 22:43:35, kulakowski wrote: > https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... > File services/media/audio/audio_pipe.cc (right): > > https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... > services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until > low water mark is reached. > On 2016/03/31 21:45:14, dalesat wrote: > > On 2016/03/31 21:32:12, kulakowski wrote: > > > > Calculating the low water mark is mostly a matter of picking a number > > > (probably > > > > a time interval) that works (prevents underflow) consistently. A lot of > > > factors > > > > can affect how low this value can be, including stuff like scheduling > > latency. > > > > In the end, we will probably come up with a hard-coded time interval that > > > always > > > > seems to work based on our testing. We might also let an application set > > this, > > > > so it can balance risk of underflow vs latency. > > > > > > It really does not seem to me that this is a single policy audio_pipe needs > to > > > create. A single hardcoded interval doesn't seem like it will be correct for > > > everyone. > > > > Sure. Is this a concern for this CL? > > Well if you have a TODO about it in the implementation of it, it seems to be to > me. Sorry, I don't know what you're asking for.
On 2016/03/31 23:19:10, dalesat wrote: > On 2016/03/31 22:43:35, kulakowski wrote: > > > https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... > > File services/media/audio/audio_pipe.cc (right): > > > > > https://codereview.chromium.org/1836953002/diff/1/services/media/audio/audio_... > > services/media/audio/audio_pipe.cc:137: // TODO(dalesat): Don't do this until > > low water mark is reached. > > On 2016/03/31 21:45:14, dalesat wrote: > > > On 2016/03/31 21:32:12, kulakowski wrote: > > > > > Calculating the low water mark is mostly a matter of picking a number > > > > (probably > > > > > a time interval) that works (prevents underflow) consistently. A lot of > > > > factors > > > > > can affect how low this value can be, including stuff like scheduling > > > latency. > > > > > In the end, we will probably come up with a hard-coded time interval > that > > > > always > > > > > seems to work based on our testing. We might also let an application set > > > this, > > > > > so it can balance risk of underflow vs latency. > > > > > > > > It really does not seem to me that this is a single policy audio_pipe > needs > > to > > > > create. A single hardcoded interval doesn't seem like it will be correct > for > > > > everyone. > > > > > > Sure. Is this a concern for this CL? > > > > Well if you have a TODO about it in the implementation of it, it seems to be > to > > me. > > Sorry, I don't know what you're asking for. I think there was some confusion on my end about who was responsible for what parts of an audio stack. I understand that you and Chris have been talking about an architecture diagram. Let's get this in, then. lgtm
Description was changed from ========== Motown: Delay audio track completion of Prime requests until a packet arrives. The audio track currently does nothing about priming (which is fine) and calls the callback immediately when a Prime request arrives (which is not fine). Ideally, it would wait to call the callback until its queue had reached its low-water mark. This CL gets partway there by waiting until the first packet arrives. This doesn't necessarily address potential underflow issues, but it does make sure we've seen at least one packet so we know how to set the rate control transform. ========== to ========== Motown: Delay audio track completion of Prime requests until a packet arrives. The audio track currently does nothing about priming (which is fine) and calls the callback immediately when a Prime request arrives (which is not fine). Ideally, it would wait to call the callback until its queue had reached its low-water mark. This CL gets partway there by waiting until the first packet arrives. This doesn't necessarily address potential underflow issues, but it does make sure we've seen at least one packet so we know how to set the rate control transform. R=kulakowski@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/fb1c0c9b02d44e74cffb6d6881f... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as fb1c0c9b02d44e74cffb6d6881f25b4f9eb50ec9 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
