Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268403006:
Use background thread for AsyncAudioDecoder (Closed)
DescriptionUse background thread for AsyncAudioDecoder
With the creation of BaseAudioContext, an additional thread dedicated to
AsyncAudioDecoder is also instantiated. This design is sub-optimal and we
can use the background thread (i.e. WorkerPool) for this purpose.
BUG=640793
TEST=NONE (all the decoder-related layout test passes)
Committed: https://crrev.com/54ff0fdf2b744504617959819fc00d38538da647
Cr-Commit-Position: refs/heads/master@{#416100}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding task speed threadhold #Patch Set 3 : Sanity check for context #
Total comments: 2
Patch Set 4 : Fixing nits after l-g-t-m #
Messages
Total messages: 46 (16 generated)
rtoy@chromium.org changed reviewers: + rtoy@chromium.org
https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:47: BackgroundTaskRunner::TaskSize taskSize = BackgroundTaskRunner::TaskSizeShortRunningTask; Why short? Decoding long files is probably not a short running task.
https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:47: BackgroundTaskRunner::TaskSize taskSize = BackgroundTaskRunner::TaskSizeShortRunningTask; TODO: The threshold for long/short task needs to be defined.
On 2016/08/24 at 23:10:19, hongchan wrote: > https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): > > https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:47: BackgroundTaskRunner::TaskSize taskSize = BackgroundTaskRunner::TaskSizeShortRunningTask; > TODO: The threshold for long/short task needs to be defined. I think something rough would be ok? Maybe if the encoded file is <= 1 MB, it's short? We can tweak this value later.
On 2016/08/25 17:12:27, Raymond Toy wrote: > On 2016/08/24 at 23:10:19, hongchan wrote: > > > https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): > > > > > https://codereview.chromium.org/2268403006/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:47: > BackgroundTaskRunner::TaskSize taskSize = > BackgroundTaskRunner::TaskSizeShortRunningTask; > > TODO: The threshold for long/short task needs to be defined. > > I think something rough would be ok? Maybe if the encoded file is <= 1 MB, it's > short? We can tweak this value later. FWIW, the canvas blob generator uses '14ms' as a threshold.
I used 512K for the threshold, but we can tweak it if this is problematic.
On 2016/08/25 21:10:53, hoch wrote: > I used 512K for the threshold, but we can tweak it if this is problematic. I also ran a performance test with the following site: http://www.scirra.com/labs/bugs/webaudiodecode/ On Mac Pro (3.5GHz, 6Core, 32GB) - This Patch (debug build): 0.17 sec M52: 0.27 sec FF: 0.32 sec
Description was changed from ========== Use background thread for AsyncAudioDecoder With the creation of BaseAudioContext, an additional thread dedicated to AsyncAudioDecoder is instantiated. This design is sub-optimal and we can use the background thread (i.e. WorkerPool) for this purpose. BUG=640793 TEST=NONE (all the decoder-related layout test passes) ========== to ========== Use background thread for AsyncAudioDecoder With the creation of BaseAudioContext, an additional thread dedicated to AsyncAudioDecoder is also instantiated. This design is sub-optimal and we can use the background thread (i.e. WorkerPool) for this purpose. BUG=640793 TEST=NONE (all the decoder-related layout test passes) ==========
hongchan@chromium.org changed reviewers: + haraken@chromium.org
haraken@ Could you take a look at the usage of BackgroundTaskRunner? The comment in base::WorkerPool says this class needs to be used carefully, so I am not 100% sure if our use case falls into the safe category.
hongchan@chromium.org changed reviewers: + junov@chromium.org
+junov@ Could you take a look at the usage of BackgroundThread here? If this is not your intended use case of BackgroundThread, please let us know!
On 2016/08/25 22:20:41, hoch wrote: > +junov@ > > Could you take a look at the usage of BackgroundThread here? If this is not your > intended use case of BackgroundThread, please let us know! Here's the more comprehensive test result: (running the test in the comment #7, averaging 10 iterations) 54.0.2840.0 (64-bit), Patch Release build = 0.0950 sec 54.0.2840.0 (64-bit), Patch Debug Build = 0.1360 sec 54.0.2840.0 (64-bit), ToT Debug Build = 0.3150 sec 52.0.2743.116 (64-bit), Stable= 0.2630 sec 54.0.2839.0 canary (64-bit), Canary = 0.2630 sec
For completeness can we have ToT release results? On Aug 25, 2016 3:45 PM, <hongchan@chromium.org> wrote: > On 2016/08/25 22:20:41, hoch wrote: > > +junov@ > > > > Could you take a look at the usage of BackgroundThread here? If this is > not > your > > intended use case of BackgroundThread, please let us know! > > Here's the more comprehensive test result: > (running the test in the comment #7, averaging 10 iterations) > > 54.0.2840.0 (64-bit), Patch Release build = 0.0950 sec > 54.0.2840.0 (64-bit), Patch Debug Build = 0.1360 sec > 54.0.2840.0 (64-bit), ToT Debug Build = 0.3150 sec > 52.0.2743.116 (64-bit), Stable= 0.2630 sec > 54.0.2839.0 canary (64-bit), Canary = 0.2630 sec > > https://codereview.chromium.org/2268403006/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
For completeness can we have ToT release results? On Aug 25, 2016 3:45 PM, <hongchan@chromium.org> wrote: > On 2016/08/25 22:20:41, hoch wrote: > > +junov@ > > > > Could you take a look at the usage of BackgroundThread here? If this is > not > your > > intended use case of BackgroundThread, please let us know! > > Here's the more comprehensive test result: > (running the test in the comment #7, averaging 10 iterations) > > 54.0.2840.0 (64-bit), Patch Release build = 0.0950 sec > 54.0.2840.0 (64-bit), Patch Debug Build = 0.1360 sec > 54.0.2840.0 (64-bit), ToT Debug Build = 0.3150 sec > 52.0.2743.116 (64-bit), Stable= 0.2630 sec > 54.0.2839.0 canary (64-bit), Canary = 0.2630 sec > > https://codereview.chromium.org/2268403006/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
haraken@chromium.org changed reviewers: + yzshen@chromium.org
On 2016/08/25 21:20:29, hoch wrote: > haraken@ > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > base::WorkerPool says this class needs to be used carefully, so I am not 100% > sure if our use case falls into the safe category. +yzshen, who added the WARNING to WorkerPool. https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... I don't think that the WorkerPool usage of decodeAsync is safe because decodeAsync can run asynchronously while the main thread is shutting down. However, I'd say that it's a bug of the WorkerPool. If we cannot use the WorkerPool, what should we use instead? :)
On 2016/08/26 02:39:29, haraken wrote: > On 2016/08/25 21:20:29, hoch wrote: > > haraken@ > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > > base::WorkerPool says this class needs to be used carefully, so I am not 100% > > sure if our use case falls into the safe category. > > +yzshen, who added the WARNING to WorkerPool. > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > I don't think that the WorkerPool usage of decodeAsync is safe because > decodeAsync can run asynchronously while the main thread is shutting down. > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > WorkerPool, what should we use instead? :) If our use case is considered not safe, I am not sure how this can safe either: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/canv... Conceptually it's similar to what we do with decoding the audio data. Perhaps its task duration is relatively short compared to our decoding task, but that does not mean the usage of BG Thread here is safe. Nonetheless, I understand that the usage of background thread is not safe. I am just curious what the justification is for the 'CanvasAsyncBlobCreator'. Can we fix the BackgroundThreadTaskRunner? The class seems very useful tasks like this. For one thing, we can make BackgroundThreadTaskRunner shut down (dropping all the ongoing tasks) when the main thread is shutting down.
On 2016/08/26 15:32:37, hoch wrote: > On 2016/08/26 02:39:29, haraken wrote: > > On 2016/08/25 21:20:29, hoch wrote: > > > haraken@ > > > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > > > base::WorkerPool says this class needs to be used carefully, so I am not > 100% > > > sure if our use case falls into the safe category. > > > > +yzshen, who added the WARNING to WorkerPool. > > > > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > > > I don't think that the WorkerPool usage of decodeAsync is safe because > > decodeAsync can run asynchronously while the main thread is shutting down. > > > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > > WorkerPool, what should we use instead? :) > > If our use case is considered not safe, I am not sure how this can safe either: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/canv... > > Conceptually it's similar to what we do with decoding the audio data. Perhaps > its task duration is relatively short compared to our decoding task, but that > does not mean the usage of BG Thread here is safe. > Nonetheless, I understand that the usage of background thread is not safe. I am > just curious what the justification is for the 'CanvasAsyncBlobCreator'. > > Can we fix the BackgroundThreadTaskRunner? The class seems very useful tasks > like this. For one thing, we can make BackgroundThreadTaskRunner shut down > (dropping all the ongoing tasks) when the main thread is shutting down. FWIW, This change brings a performance boost (~3x) while removing a redundant thread, so this is definitely worth it. A simple comparison between different builds (google corp account required): https://docs.google.com/spreadsheets/d/1IYO6ri659oAuJROtmnWVNBnOTEIXj7rSecU-2...
On 2016/08/26 at 15:41:22, hongchan wrote: > On 2016/08/26 15:32:37, hoch wrote: > > On 2016/08/26 02:39:29, haraken wrote: > > > On 2016/08/25 21:20:29, hoch wrote: > > > > haraken@ > > > > > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > > > > base::WorkerPool says this class needs to be used carefully, so I am not > > 100% > > > > sure if our use case falls into the safe category. > > > > > > +yzshen, who added the WARNING to WorkerPool. > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > > > > > I don't think that the WorkerPool usage of decodeAsync is safe because > > > decodeAsync can run asynchronously while the main thread is shutting down. > > > > > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > > > WorkerPool, what should we use instead? :) > > > > If our use case is considered not safe, I am not sure how this can safe either: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/canv... > > > > Conceptually it's similar to what we do with decoding the audio data. Perhaps > > its task duration is relatively short compared to our decoding task, but that > > does not mean the usage of BG Thread here is safe. > > Nonetheless, I understand that the usage of background thread is not safe. I am > > just curious what the justification is for the 'CanvasAsyncBlobCreator'. > > > > Can we fix the BackgroundThreadTaskRunner? The class seems very useful tasks > > like this. For one thing, we can make BackgroundThreadTaskRunner shut down > > (dropping all the ongoing tasks) when the main thread is shutting down. > > FWIW, This change brings a performance boost (~3x) while removing a redundant thread, so this is definitely worth it. > > A simple comparison between different builds (google corp account required): > https://docs.google.com/spreadsheets/d/1IYO6ri659oAuJROtmnWVNBnOTEIXj7rSecU-2... Since the amount of work for decoding a file is the same independent of the kind of thread being used, I have to wonder why the WorkerPool is so much faster. Do they start up faster? Are multiple threads running? What makes it so much faster? And in the current code, we could be lazy about it and create the decoder thread on demand instead of when the context is created, as we currently do today.
hongchan@chromium.org changed reviewers: + esprehn@chromium.org
On 2016/08/26 16:21:01, Raymond Toy wrote: > On 2016/08/26 at 15:41:22, hongchan wrote: > > On 2016/08/26 15:32:37, hoch wrote: > > > On 2016/08/26 02:39:29, haraken wrote: > > > > On 2016/08/25 21:20:29, hoch wrote: > > > > > haraken@ > > > > > > > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment > in > > > > > base::WorkerPool says this class needs to be used carefully, so I am > not > > > 100% > > > > > sure if our use case falls into the safe category. > > > > > > > > +yzshen, who added the WARNING to WorkerPool. > > > > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > > > > > > > I don't think that the WorkerPool usage of decodeAsync is safe because > > > > decodeAsync can run asynchronously while the main thread is shutting down. > > > > > > > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > > > > WorkerPool, what should we use instead? :) > > > > > > If our use case is considered not safe, I am not sure how this can safe > either: > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/canv... > > > > > > Conceptually it's similar to what we do with decoding the audio data. > Perhaps > > > its task duration is relatively short compared to our decoding task, but > that > > > does not mean the usage of BG Thread here is safe. > > > Nonetheless, I understand that the usage of background thread is not safe. I > am > > > just curious what the justification is for the 'CanvasAsyncBlobCreator'. > > > > > > Can we fix the BackgroundThreadTaskRunner? The class seems very useful tasks > > > like this. For one thing, we can make BackgroundThreadTaskRunner shut down > > > (dropping all the ongoing tasks) when the main thread is shutting down. > > > > FWIW, This change brings a performance boost (~3x) while removing a redundant > thread, so this is definitely worth it. > > > > A simple comparison between different builds (google corp account required): > > > https://docs.google.com/spreadsheets/d/1IYO6ri659oAuJROtmnWVNBnOTEIXj7rSecU-2... > > Since the amount of work for decoding a file is the same independent of the kind > of thread being used, > I have to wonder why the WorkerPool is so much faster. Do they start up faster? > Are multiple threads running? > What makes it so much faster? > > And in the current code, we could be lazy about it and create the decoder thread > on demand instead of when the > context is created, as we currently do today. +esprehn@ What would be the best option for this problem? We really like to use the background thread for this because: 1) An extra thread lying around doing almost nothing is wasteful. 2) Creating a thread whenever decodeAudioData() is called will cause too much overhead for starting up/tearing down. We also have a similar issue with OfflineAudioContext, which creates a WebThread on demand. Some sorts of Worker or Thread Pool in Blink would be really nice. WDYT?
On 2016/08/26 02:39:29, haraken wrote: > On 2016/08/25 21:20:29, hoch wrote: > > haraken@ > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > > base::WorkerPool says this class needs to be used carefully, so I am not 100% > > sure if our use case falls into the safe category. > > +yzshen, who added the WARNING to WorkerPool. > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > I don't think that the WorkerPool usage of decodeAsync is safe because > decodeAsync can run asynchronously while the main thread is shutting down. > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > WorkerPool, what should we use instead? :) Hi, haraken. I didn't add that comment. It was added back in 2010: https://chromium.googlesource.com/chromium/src/+/6c710ee20601c5afeb5640498206... I landed a feature to allow WorkerPool to shutdown cleanly, so I updated the comment. But later that feature was no longer used by our project, so I reverted it as well as the comment change. That is why you saw me on the blame list. Generally speaking, we don't wait for WorkerPool tasks to complete during shutdown, so they must not depend on other things that may be destroyed during shutdown. With your recent work of removing graceful shutdown, is this no longer a concern?
On 2016/08/26 15:32:37, hoch wrote: > On 2016/08/26 02:39:29, haraken wrote: > > On 2016/08/25 21:20:29, hoch wrote: > > > haraken@ > > > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > > > base::WorkerPool says this class needs to be used carefully, so I am not > 100% > > > sure if our use case falls into the safe category. > > > > +yzshen, who added the WARNING to WorkerPool. > > > > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > > > I don't think that the WorkerPool usage of decodeAsync is safe because > > decodeAsync can run asynchronously while the main thread is shutting down. > > > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > > WorkerPool, what should we use instead? :) > > If our use case is considered not safe, I am not sure how this can safe either: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/canv... > > Conceptually it's similar to what we do with decoding the audio data. Perhaps > its task duration is relatively short compared to our decoding task, but that > does not mean the usage of BG Thread here is safe. > Nonetheless, I understand that the usage of background thread is not safe. I am > just curious what the justification is for the 'CanvasAsyncBlobCreator'. > > Can we fix the BackgroundThreadTaskRunner? The class seems very useful tasks > like this. For one thing, we can make BackgroundThreadTaskRunner shut down > (dropping all the ongoing tasks) when the main thread is shutting down. You're right. CanvasAsyncBlobCreator wouldn't be safe either. Also I totally agree with you that we should fix BackgroundThreadTaskRunner or WorkerPool and get rid of the restriction about shutdown.
On 2016/08/26 16:21:01, Raymond Toy wrote: > On 2016/08/26 at 15:41:22, hongchan wrote: > > On 2016/08/26 15:32:37, hoch wrote: > > > On 2016/08/26 02:39:29, haraken wrote: > > > > On 2016/08/25 21:20:29, hoch wrote: > > > > > haraken@ > > > > > > > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment > in > > > > > base::WorkerPool says this class needs to be used carefully, so I am > not > > > 100% > > > > > sure if our use case falls into the safe category. > > > > > > > > +yzshen, who added the WARNING to WorkerPool. > > > > > > > > > > > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > > > > > > > I don't think that the WorkerPool usage of decodeAsync is safe because > > > > decodeAsync can run asynchronously while the main thread is shutting down. > > > > > > > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > > > > WorkerPool, what should we use instead? :) > > > > > > If our use case is considered not safe, I am not sure how this can safe > either: > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/canv... > > > > > > Conceptually it's similar to what we do with decoding the audio data. > Perhaps > > > its task duration is relatively short compared to our decoding task, but > that > > > does not mean the usage of BG Thread here is safe. > > > Nonetheless, I understand that the usage of background thread is not safe. I > am > > > just curious what the justification is for the 'CanvasAsyncBlobCreator'. > > > > > > Can we fix the BackgroundThreadTaskRunner? The class seems very useful tasks > > > like this. For one thing, we can make BackgroundThreadTaskRunner shut down > > > (dropping all the ongoing tasks) when the main thread is shutting down. > > > > FWIW, This change brings a performance boost (~3x) while removing a redundant > thread, so this is definitely worth it. > > > > A simple comparison between different builds (google corp account required): > > > https://docs.google.com/spreadsheets/d/1IYO6ri659oAuJROtmnWVNBnOTEIXj7rSecU-2... > > Since the amount of work for decoding a file is the same independent of the kind > of thread being used, > I have to wonder why the WorkerPool is so much faster. Do they start up faster? > Are multiple threads running? > What makes it so much faster? > > And in the current code, we could be lazy about it and create the decoder thread > on demand instead of when the > context is created, as we currently do today. Threads in the WorkerPool are on standby when you post a task. There is no start-up cost in common cases.
On 2016/08/26 21:30:14, yzshen1 wrote: > On 2016/08/26 02:39:29, haraken wrote: > > On 2016/08/25 21:20:29, hoch wrote: > > > haraken@ > > > > > > Could you take a look at the usage of BackgroundTaskRunner? The comment in > > > base::WorkerPool says this class needs to be used carefully, so I am not > 100% > > > sure if our use case falls into the safe category. > > > > +yzshen, who added the WARNING to WorkerPool. > > > > > https://cs.chromium.org/chromium/src/base/threading/worker_pool.h?sq=package:... > > > > I don't think that the WorkerPool usage of decodeAsync is safe because > > decodeAsync can run asynchronously while the main thread is shutting down. > > > > However, I'd say that it's a bug of the WorkerPool. If we cannot use the > > WorkerPool, what should we use instead? :) > > Hi, haraken. > > I didn't add that comment. It was added back in 2010: > https://chromium.googlesource.com/chromium/src/+/6c710ee20601c5afeb5640498206... > > I landed a feature to allow WorkerPool to shutdown cleanly, so I updated the > comment. > But later that feature was no longer used by our project, so I reverted it as > well as the comment change. > That is why you saw me on the blame list. > > Generally speaking, we don't wait for WorkerPool tasks to complete during > shutdown, so they must not depend on other things that may be destroyed during > shutdown. With your recent work of removing graceful shutdown, is this no longer > a concern? Yeah, that would be a right direction to go. Currently I've only removed blink::shutdown, so the WorkerPool is not yet safe. We should remove more shutdown code enough to make the WorkerPool safe. That said, I don't think we need to block this CL by the shutdown refactoring. It's indeed true that the current WorkerPool is *potentially* not safe but I think it's extremely rare to the issue in practice (i.e., it happens only when WebAudio is running the decoder and touching some specific objects while the renderer is shutting down). I think it's okay to land this CL for now and then fix the safety issue later.
Yeah I'd say lets go forward with this CL.
hongchan@chromium.org changed reviewers: - junov@chromium.org, yzshen@chromium.org
That sounds good. Then what else needs to be done here? We can check if |context| still exists when posting task back and forth, but I don't think it really contributes to anything because AsyncAudioDecoder is a member of BaseAudioContext. So when the context gets destroyed, the async decoder will also be gone.
LGTM on my side
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm 2
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
esprehn@chromium.org changed reviewers: + junov@chromium.org
lgtm https://codereview.chromium.org/2268403006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): https://codereview.chromium.org/2268403006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:44: unsigned TaskSizeThresholdInByte = 512000; static and prefix with "k"
On 2016/08/30 21:40:16, esprehn wrote: > lgtm > > https://codereview.chromium.org/2268403006/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): > > https://codereview.chromium.org/2268403006/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:44: unsigned > TaskSizeThresholdInByte = 512000; > static and prefix with "k" Thanks everyone! I will land the CL after the final discussion with rtoy@.
lgtm https://codereview.chromium.org/2268403006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp (right): https://codereview.chromium.org/2268403006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/AsyncAudioDecoder.cpp:44: unsigned TaskSizeThresholdInByte = 512000; On 2016/08/30 at 21:40:16, esprehn wrote: > static and prefix with "k" And const.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, esprehn@chromium.org, junov@chromium.org, rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2268403006/#ps60001 (title: "Fixing nits after l-g-t-m")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use background thread for AsyncAudioDecoder With the creation of BaseAudioContext, an additional thread dedicated to AsyncAudioDecoder is also instantiated. This design is sub-optimal and we can use the background thread (i.e. WorkerPool) for this purpose. BUG=640793 TEST=NONE (all the decoder-related layout test passes) ========== to ========== Use background thread for AsyncAudioDecoder With the creation of BaseAudioContext, an additional thread dedicated to AsyncAudioDecoder is also instantiated. This design is sub-optimal and we can use the background thread (i.e. WorkerPool) for this purpose. BUG=640793 TEST=NONE (all the decoder-related layout test passes) ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use background thread for AsyncAudioDecoder With the creation of BaseAudioContext, an additional thread dedicated to AsyncAudioDecoder is also instantiated. This design is sub-optimal and we can use the background thread (i.e. WorkerPool) for this purpose. BUG=640793 TEST=NONE (all the decoder-related layout test passes) ========== to ========== Use background thread for AsyncAudioDecoder With the creation of BaseAudioContext, an additional thread dedicated to AsyncAudioDecoder is also instantiated. This design is sub-optimal and we can use the background thread (i.e. WorkerPool) for this purpose. BUG=640793 TEST=NONE (all the decoder-related layout test passes) Committed: https://crrev.com/54ff0fdf2b744504617959819fc00d38538da647 Cr-Commit-Position: refs/heads/master@{#416100} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/54ff0fdf2b744504617959819fc00d38538da647 Cr-Commit-Position: refs/heads/master@{#416100} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
