Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1013923002:
Fix MSE GC, make it less aggressive, more spec-compliant (Blink CL) (Closed)
|
Created:
5 years, 9 months ago by servolk Modified:
5 years, 4 months ago CC:
blink-reviews, feature-media-reviews_chromium.org, dglazkov+blink, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix MSE GC, make it less aggressive, more spec-compliant (Blink CL)
Provided interface in ChunkDemuxer/WebSourceBuffer to allow blink to
invoke "3.5.12 Coded Frame Eviction Algorithm"
https://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction
This allows us to detect when buffer is full, even after GC
and throw QuotaExceededErr exception as MSE spec prescribes.
See step #6 in section 3.5.4 at
https://w3c.github.io/media-source/#sourcebuffer-prepare-append
Related Chromium CL: https://codereview.chromium.org/1008463002/
BUG=421694
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200989
Patch Set 1 #Patch Set 2 : Rebase to ToT #Patch Set 3 : Added a basic test for evict frames functionality #
Total comments: 10
Patch Set 4 : Added QuotaExceededErr test case for appendStream #Patch Set 5 : Added decodeError parameter for appendError function #
Total comments: 4
Patch Set 6 : Refactoring #Patch Set 7 : Removed TODO, implemented calling m_source->endOfStream #Patch Set 8 : Pass currentTime from blink level, instead of using GetMediaTimeCB #
Total comments: 30
Patch Set 9 : CR feedback #
Total comments: 22
Patch Set 10 : Increase test performance by using larger input file + CR feedback #Patch Set 11 : Split unit tests for appendBuffer/Stream into separate files #Patch Set 12 : Pass new data size into evictCodedFrames #Patch Set 13 : Rebase to ToT + a bit more logging #Patch Set 14 : Use smaller data file for separate tests #
Total comments: 5
Patch Set 15 : Split LayoutTests into a separate CL #Patch Set 16 : Moved evictCodedFrames invocation to SourceBuffer::didReceiveDataForClient #Patch Set 17 : Adjust comments #Patch Set 18 : m_loader might be released before didFail, if appendStream failed due to full buffer #
Messages
Total messages: 46 (4 generated)
servolk@chromium.org changed reviewers: - damienv@chromium.org
Looks good, can you take a look too, wolenetz@? https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... File Source/modules/mediasource/SourceBuffer.cpp (left): https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:718: // 12. Loop Done: Set the updating attribute to false. Can you also renumber the remaining steps, this one starting at 16? https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:536: exceptionState.throwDOMException(QuotaExceededError, "The SourceBuffer is full, and cannot free space to append additional buffers."); This is the new bit covered by the test. https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:667: exceptionState.throwDOMException(QuotaExceededError, "The SourceBuffer is full, and cannot free space to append stream."); Is it possible to test the appendStream() case? https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:726: appendError(); Is it possible to reach this case and write a test for the events that are fired as a result? It seems like a strange case, might it be dead code in both spec and implementation? https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:750: void SourceBuffer::appendError() Per spec there's a "decode error" argument that's always false where this is called, but is there a place in "Initialization Segment Received" where there should be a TODO to call appendError(true) or similar?
https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:536: exceptionState.throwDOMException(QuotaExceededError, "The SourceBuffer is full, and cannot free space to append additional buffers."); On 2015/06/01 14:37:27, philipj wrote: > This is the new bit covered by the test. Acknowledged. https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:667: exceptionState.throwDOMException(QuotaExceededError, "The SourceBuffer is full, and cannot free space to append stream."); On 2015/06/01 14:37:27, philipj wrote: > Is it possible to test the appendStream() case? Done. But actually I think the test is hitting the other case for appendStream (the one in appendStreamDone). Here we run evictCodedFrames before we started appending data, so this one should succeed. https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:726: appendError(); On 2015/06/01 14:37:27, philipj wrote: > Is it possible to reach this case and write a test for the events that are fired > as a result? It seems like a strange case, might it be dead code in both spec > and implementation? See above, I think the new test that I've added is hitting this code path, since we are doing evictCodedFrames after appending stream data here and we might be over memory limit and GC still might fail if data is not consumed by the media pipeline (which will be the case during test, since the test doesn't start playback, only keeps appending data). https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:750: void SourceBuffer::appendError() On 2015/06/01 14:37:27, philipj wrote: > Per spec there's a "decode error" argument that's always false where this is > called, but is there a place in "Initialization Segment Received" where there > should be a TODO to call appendError(true) or similar? You are right. I've added a decodeError parameter, but I'm not sure how to call endOfStream here, since it expects ExceptionState as an input parameter and we don't have it here. Should we pass exception state from the point where async call is initiated? Does Javascript expect this async method to throw exception in the async part of the method?
https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/40001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:667: exceptionState.throwDOMException(QuotaExceededError, "The SourceBuffer is full, and cannot free space to append stream."); On 2015/06/02 19:24:31, servolk wrote: > On 2015/06/01 14:37:27, philipj wrote: > > Is it possible to test the appendStream() case? > > Done. But actually I think the test is hitting the other case for appendStream > (the one in appendStreamDone). Here we run evictCodedFrames before we started > appending data, so this one should succeed. Actually, no, I was wrong. The new test case that I've added hits both code paths. I've added logging in all three places where we invoke evictCodedFrames and re-ran the tests and I can see these log messages from all three places in stderr output. Here is how it works: the test keeps appending the same stream to SB over and over again and eventually the SB fills up and we hit the appendError code path in SourceBuffer::appendStreamDone. But then the test doesn't stop, it ignores the error event and sees updateend event, and then tries once again to append more data to SB that's already full. Since now the buffer is already full at the beginning of append operation, we hit this code path right here and throw QuotaExceededErr. The test catches the QuotaExceededErr exception and completes successfully.
Thanks, code-wise I think this looks right. The testing seems a bit tricky, if you'd like to investigate writing this is a unit test with a mock WebSourceBuffer you may find that it's easier to get the precise error case under testing, and the tests would probably run 1000x faster. https://codereview.chromium.org/1013923002/diff/80001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:81: }, 'Calling appendStream repeatedly should fill up the buffer and throw a QuotaExceededError when buffer is full.', {timeout: 45000}); This is a pretty extreme timeout, how long does this test usually take? https://codereview.chromium.org/1013923002/diff/80001/Source/modules/mediasou... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/80001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:767: // TODO(servolk): endOfStream expects ExceptionState parameter, but it is The exception to worry about is the throwExceptionIfClosedOrUpdating(isOpen(), isUpdating(), exceptionState) in MediaSource::endOfStreamInternal. We're definitely not updating here and if it possible to be closed that would be a spec problem, since there's nothing to do with the exception. If you poke around a bit an conclude that the exception cannot occur here, just pass ASSERT_NO_EXCEPTION for the ExceptionState&.
On 2015/06/03 12:43:32, philipj wrote: > Thanks, code-wise I think this looks right. The testing seems a bit tricky, if > you'd like to investigate writing this is a unit test with a mock > WebSourceBuffer you may find that it's easier to get the precise error case > under testing, and the tests would probably run 1000x faster. That's true, though it would expand the scope of media-source-specific layout tests to places (like media/...) other than just under http/tests/media/media-source. Or do I misunderstand? This isn't a blocker, but it does add some complexity to integrating with interop web-platform-tests.
Looks pretty good. It needs the ExceptionState parameter fixed per philipj's reply. Thanks for working on this!
On 2015/06/04 00:56:26, wolenetz wrote: > On 2015/06/03 12:43:32, philipj wrote: > > Thanks, code-wise I think this looks right. The testing seems a bit tricky, if > > you'd like to investigate writing this is a unit test with a mock > > WebSourceBuffer you may find that it's easier to get the precise error case > > under testing, and the tests would probably run 1000x faster. > > That's true, though it would expand the scope of media-source-specific layout > tests to places (like media/...) other than just under > http/tests/media/media-source. Or do I misunderstand? This isn't a blocker, but > it does add some complexity to integrating with interop web-platform-tests. Oh, by unit tests I mean C++ gtest/gmock tests, so that couldn't be usptreamed to web-platform-tests under any scenario. As long as JavaScript tests run fast and aren't flaky I prefer that, but this might just be a case where a C++ test would work better.
On 2015/06/04 11:19:14, philipj wrote: > On 2015/06/04 00:56:26, wolenetz wrote: > > On 2015/06/03 12:43:32, philipj wrote: > > > Thanks, code-wise I think this looks right. The testing seems a bit tricky, > if > > > you'd like to investigate writing this is a unit test with a mock > > > WebSourceBuffer you may find that it's easier to get the precise error case > > > under testing, and the tests would probably run 1000x faster. > > > > That's true, though it would expand the scope of media-source-specific layout > > tests to places (like media/...) other than just under > > http/tests/media/media-source. Or do I misunderstand? This isn't a blocker, > but > > it does add some complexity to integrating with interop web-platform-tests. > > Oh, by unit tests I mean C++ gtest/gmock tests, so that couldn't be usptreamed > to web-platform-tests under any scenario. > > As long as JavaScript tests run fast and aren't flaky I prefer that, but this > might just be a case where a C++ test would work better. Ah. I did misunderstand. I, too, prefer a fast running JS layout test (since similar could be used upstream to show MSE interop). If it's too slow-running for us downstream to include in bots all the time, how best could we retain the test for occasional running? (A mocked WebSourceBuffer in a blink unit test wouldn't test the integrated memory quota exceeded behavior, but would test the SourceBuffer logic.) If we chose to only occasionally run the JS test, I agree we would at least need the unit test too.
On 2015/06/04 18:34:49, wolenetz wrote: > On 2015/06/04 11:19:14, philipj wrote: > > On 2015/06/04 00:56:26, wolenetz wrote: > > > On 2015/06/03 12:43:32, philipj wrote: > > > > Thanks, code-wise I think this looks right. The testing seems a bit > tricky, > > if > > > > you'd like to investigate writing this is a unit test with a mock > > > > WebSourceBuffer you may find that it's easier to get the precise error > case > > > > under testing, and the tests would probably run 1000x faster. > > > > > > That's true, though it would expand the scope of media-source-specific > layout > > > tests to places (like media/...) other than just under > > > http/tests/media/media-source. Or do I misunderstand? This isn't a blocker, > > but > > > it does add some complexity to integrating with interop web-platform-tests. > > > > Oh, by unit tests I mean C++ gtest/gmock tests, so that couldn't be usptreamed > > to web-platform-tests under any scenario. > > > > As long as JavaScript tests run fast and aren't flaky I prefer that, but this > > might just be a case where a C++ test would work better. > > Ah. I did misunderstand. I, too, prefer a fast running JS layout test (since > similar could be used upstream to show MSE interop). If it's too slow-running > for us downstream to include in bots all the time, how best could we retain the > test for occasional running? (A mocked WebSourceBuffer in a blink unit test > wouldn't test the integrated memory quota exceeded behavior, but would test the > SourceBuffer logic.) If we chose to only occasionally run the JS test, I agree > we would at least need the unit test too. If a test is too slow for us then I think it would be too slow for web-platform-tests too. After all, the goal is to run wpt as part of Blink, Gecko and other's regular testing, so if we put in really slow tests that'll create trouble for Mozilla who are farther along than we are with wpt integration. My colleague davve@ has actually been asked to fix one of his tests in wpt that's taking a bit too long to run. In this case, I would investigate the fundamental reason that this is so slow. It can't be limited by the speed of memory itself, because to write hundreds of megabytes of memory should be a very quick affair. How many updateend events are being fired? Could it be that each step appends so little that the time is dominated by the overhead of firing the events? If there aren't that many events, perhaps there's some artificial amount of delay caused by excessive thread hopping or a silly timeout hidden somewhere?
On 2015/06/04 18:45:04, philipj wrote: > On 2015/06/04 18:34:49, wolenetz wrote: > > On 2015/06/04 11:19:14, philipj wrote: > > > On 2015/06/04 00:56:26, wolenetz wrote: > > > > On 2015/06/03 12:43:32, philipj wrote: > > > > > Thanks, code-wise I think this looks right. The testing seems a bit > > tricky, > > > if > > > > > you'd like to investigate writing this is a unit test with a mock > > > > > WebSourceBuffer you may find that it's easier to get the precise error > > case > > > > > under testing, and the tests would probably run 1000x faster. > > > > > > > > That's true, though it would expand the scope of media-source-specific > > layout > > > > tests to places (like media/...) other than just under > > > > http/tests/media/media-source. Or do I misunderstand? This isn't a > blocker, > > > but > > > > it does add some complexity to integrating with interop > web-platform-tests. > > > > > > Oh, by unit tests I mean C++ gtest/gmock tests, so that couldn't be > usptreamed > > > to web-platform-tests under any scenario. > > > > > > As long as JavaScript tests run fast and aren't flaky I prefer that, but > this > > > might just be a case where a C++ test would work better. > > > > Ah. I did misunderstand. I, too, prefer a fast running JS layout test (since > > similar could be used upstream to show MSE interop). If it's too slow-running > > for us downstream to include in bots all the time, how best could we retain > the > > test for occasional running? (A mocked WebSourceBuffer in a blink unit test > > wouldn't test the integrated memory quota exceeded behavior, but would test > the > > SourceBuffer logic.) If we chose to only occasionally run the JS test, I agree > > we would at least need the unit test too. > > If a test is too slow for us then I think it would be too slow for > web-platform-tests too. After all, the goal is to run wpt as part of Blink, > Gecko and other's regular testing, so if we put in really slow tests that'll > create trouble for Mozilla who are farther along than we are with wpt > integration. My colleague davve@ has actually been asked to fix one of his tests > in wpt that's taking a bit too long to run. > > In this case, I would investigate the fundamental reason that this is so slow. > It can't be limited by the speed of memory itself, because to write hundreds of > megabytes of memory should be a very quick affair. How many updateend events are > being fired? Could it be that each step appends so little that the time is > dominated by the overhead of firing the events? If there aren't that many > events, perhaps there's some artificial amount of delay caused by excessive > thread hopping or a silly timeout hidden somewhere? Hi, yeah, for some reason the appendStream test is very slow, I think that's either because appendStream code path is itself somewhat slower than appendBuffer in blink code (although I can't think of good reasons why that could be the case), or because appendStream test has to re-download stream repeatedly via XHR (since after we append the downloaded stream once, it becomes neutered and we can't re-use it again). Both tests use audio mime types, so the amount of buffered data that we need to reach in both cases is 12Mb (that's the default memory limit for audio SBS, see media/filters/source_buffer_platform.cc). Unfortunately ChunkDemuxer::SetMemoryLimits that is used by native tests seems to be inaccessible from JS level, so if we want this to be LayoutTest we'll have to live with longer test times. appendStream layout test currently takes ~18 seconds on my standard Chrome developer workstation (HP Z620 with Intel(R) Xeon(R) CPU E5-2690 0 @ 2.90GHz). I'll try adding some more logging to figure out what takes so long.
On 2015/06/04 18:34:49, wolenetz wrote: > On 2015/06/04 11:19:14, philipj wrote: > > On 2015/06/04 00:56:26, wolenetz wrote: > > > On 2015/06/03 12:43:32, philipj wrote: > > > > Thanks, code-wise I think this looks right. The testing seems a bit > tricky, > > if > > > > you'd like to investigate writing this is a unit test with a mock > > > > WebSourceBuffer you may find that it's easier to get the precise error > case > > > > under testing, and the tests would probably run 1000x faster. > > > > > > That's true, though it would expand the scope of media-source-specific > layout > > > tests to places (like media/...) other than just under > > > http/tests/media/media-source. Or do I misunderstand? This isn't a blocker, > > but > > > it does add some complexity to integrating with interop web-platform-tests. > > > > Oh, by unit tests I mean C++ gtest/gmock tests, so that couldn't be usptreamed > > to web-platform-tests under any scenario. > > > > As long as JavaScript tests run fast and aren't flaky I prefer that, but this > > might just be a case where a C++ test would work better. > > Ah. I did misunderstand. I, too, prefer a fast running JS layout test (since > similar could be used upstream to show MSE interop). If it's too slow-running > for us downstream to include in bots all the time, how best could we retain the > test for occasional running? (A mocked WebSourceBuffer in a blink unit test > wouldn't test the integrated memory quota exceeded behavior, but would test the > SourceBuffer logic.) If we chose to only occasionally run the JS test, I agree > we would at least need the unit test too. Re this - there's not much logic in WebSourceBuffer. Our main goal here (in blink CL) is to test that QuotaExceededErr is thrown as expected when GC fails. The main test for SourceBuffer/GC logic is in the other part of this CL (see source_buffer_stream_unittest.cc changes in https://codereview.chromium.org/1008463002/).
https://codereview.chromium.org/1013923002/diff/80001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/80001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:81: }, 'Calling appendStream repeatedly should fill up the buffer and throw a QuotaExceededError when buffer is full.', {timeout: 45000}); On 2015/06/03 12:43:32, philipj wrote: > This is a pretty extreme timeout, how long does this test usually take? Currently it slightly less than 18 seconds on my standard Chromium dev workstation (HP Z620 with Intel(R) Xeon(R) CPU E5-2690 0 @ 2.90GHz). For some reason appends through the appendStream API are taking much longer (or perhaps it's the fact that we repeatedly have to download data through XHR, not sure atm). https://codereview.chromium.org/1013923002/diff/80001/Source/modules/mediasou... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/80001/Source/modules/mediasou... Source/modules/mediasource/SourceBuffer.cpp:767: // TODO(servolk): endOfStream expects ExceptionState parameter, but it is On 2015/06/03 12:43:32, philipj wrote: > The exception to worry about is the throwExceptionIfClosedOrUpdating(isOpen(), > isUpdating(), exceptionState) in MediaSource::endOfStreamInternal. We're > definitely not updating here and if it possible to be closed that would be a > spec problem, since there's nothing to do with the exception. If you poke around > a bit an conclude that the exception cannot occur here, just pass > ASSERT_NO_EXCEPTION for the ExceptionState&. Done.
Sorry, this has been sitting idle for a while. Is it now waiting for final review, or are you investigating why the test is so slow?
On 2015/06/29 07:29:37, philipj wrote: > Sorry, this has been sitting idle for a while. Is it now waiting for final > review, or are you investigating why the test is so slow? I've made some changes to both this CL and related Chromium CL https://codereview.chromium.org/1008463002/ to pass current playback position down from blink level into the evictCodedFrames algorithm on SourceBufferStream level and I was waiting for wolenetz@ to take a look at those changes. In the meantime I've also made some adjustments to this CL (patchsets 6-8): Moved "prepare append algorithm" into a separate prepareAppend method to remove some code duplicated between appendStreamInternal and appendBufferInternal. Then in PS 7-8 I've also added MediaSource::getHTMLMediaElement - which returns HTMLMediaElement that the MediaSource is attached to. This will allow us to obtain HTMLMediaElement::currentTime, which is going to be used for GC algorithm, and also will allow us to check HTMLMediaElement.error property value in the step 3 of prepareAppend algorithm as described in MSE spec. So I guess it would make sense for you also to take one more look at this blink CL. Re performance - I still don't know why the test is so slow. I was hoping to make it fast by limiting the size of the SourceBuffer (CL https://codereview.chromium.org/1182183004/) but ddorwin@ objected to that. So now I will either need a better approach to set SourceBuffer memory limits, or try to do some profiling of the test. Unfortunately I'm not very familiar with blink, can you recommend a way to analyze performance of a layout test? Is there a way to run layout test with profiling and/or tracing turned on? Or is there a way to run a layout test by starting blink_tests with some flag and navigating to the test page? I've also added a few more tracing markers in patchset #6 here, hopefully that will allow us to get a better understanding what is taking so much time during appendStream test.
On 2015/06/29 23:49:07, servolk wrote: > On 2015/06/29 07:29:37, philipj wrote: > > Sorry, this has been sitting idle for a while. Is it now waiting for final > > review, or are you investigating why the test is so slow? > > I've made some changes to both this CL and related Chromium CL > https://codereview.chromium.org/1008463002/ to pass current playback position > down from blink level into the evictCodedFrames algorithm on SourceBufferStream > level and I was waiting for wolenetz@ to take a look at those changes. In the > meantime I've also made some adjustments to this CL (patchsets 6-8): > Moved "prepare append algorithm" into a separate prepareAppend method to remove > some code duplicated between appendStreamInternal and appendBufferInternal. Then > in PS 7-8 I've also added MediaSource::getHTMLMediaElement - which returns > HTMLMediaElement that the MediaSource is attached to. This will allow us to > obtain HTMLMediaElement::currentTime, which is going to be used for GC > algorithm, and also will allow us to check HTMLMediaElement.error property value > in the step 3 of prepareAppend algorithm as described in MSE spec. So I guess it > would make sense for you also to take one more look at this blink CL. > Re performance - I still don't know why the test is so slow. I was hoping to > make it fast by limiting the size of the SourceBuffer (CL > https://codereview.chromium.org/1182183004/) but ddorwin@ objected to that. So > now I will either need a better approach to set SourceBuffer memory limits, or > try to do some profiling of the test. Unfortunately I'm not very familiar with > blink, can you recommend a way to analyze performance of a layout test? Is there > a way to run layout test with profiling and/or tracing turned on? Or is there a > way to run a layout test by starting blink_tests with some flag and navigating > to the test page? I've also added a few more tracing markers in patchset #6 > here, hopefully that will allow us to get a better understanding what is taking > so much time during appendStream test. Oh, yes, btw, I've found a '--profile' option for run-webkit-tests, but it's not working on my machine (I'm running standard Goobuntu Linux with 3.13.0-45-generic kernel and I have just installed linux-tools-3.13.0-45-generic): ninja -C out/Release/ blink_tests && ./third_party/WebKit/Tools/Scripts/run-webkit-tests --profile --time-out 20000 -v http/tests/media/media-source/mediasource-evi\* ninja: Entering directory `out/Release/' ninja: no work to do. Using port 'linux-x86_64' Test configuration: <lucid, x86_64, release> View the test results at file:///media/ssd850/chromium/src/webkit/Release/layout-test-results/results.html View the archived results dashboard at file:///media/ssd850/chromium/src/webkit/Release/layout-test-results/dashboard.html Baseline search path: linux -> win -> generic Using Release build Pixel tests enabled Regular timeout: 20000, slow test timeout: 100000 Command line: /media/ssd850/chromium/src/out/Release/content_shell --run-layout-test --enable-slimming-paint --enable-crash-reporter --crash-dumps-dir=/media/ssd850/chromium/src/out/Release/crash-dumps - Found 1 test; running 1, skipping 0. Running 1 content_shell. [0/1] http/tests/media/media-source/mediasource-evict-frames.htmlcallchain: Unknown --call-graph option value: --pid usage: perf record [<options>] [<command>] or: perf record [<options>] -- <command> [<options>] --call-graph <mode[,dump_size]> setup and enables call-graph (stack chain/backtrace) recording: fp dwarf [1/1] http/tests/media/media-source/mediasource-evict-frames.html passed 'perf record' failed (exit code: 129), can't process results:
On 2015/06/29 23:49:07, servolk wrote: > On 2015/06/29 07:29:37, philipj wrote: > > Sorry, this has been sitting idle for a while. Is it now waiting for final > > review, or are you investigating why the test is so slow? > > I've made some changes to both this CL and related Chromium CL > https://codereview.chromium.org/1008463002/ to pass current playback position > down from blink level into the evictCodedFrames algorithm on SourceBufferStream > level and I was waiting for wolenetz@ to take a look at those changes. In the > meantime I've also made some adjustments to this CL (patchsets 6-8): > Moved "prepare append algorithm" into a separate prepareAppend method to remove > some code duplicated between appendStreamInternal and appendBufferInternal. Then > in PS 7-8 I've also added MediaSource::getHTMLMediaElement - which returns > HTMLMediaElement that the MediaSource is attached to. This will allow us to > obtain HTMLMediaElement::currentTime, which is going to be used for GC > algorithm, and also will allow us to check HTMLMediaElement.error property value > in the step 3 of prepareAppend algorithm as described in MSE spec. So I guess it > would make sense for you also to take one more look at this blink CL. > Re performance - I still don't know why the test is so slow. I was hoping to > make it fast by limiting the size of the SourceBuffer (CL > https://codereview.chromium.org/1182183004/) but ddorwin@ objected to that. So > now I will either need a better approach to set SourceBuffer memory limits, or > try to do some profiling of the test. Unfortunately I'm not very familiar with > blink, can you recommend a way to analyze performance of a layout test? Is there > a way to run layout test with profiling and/or tracing turned on? Or is there a > way to run a layout test by starting blink_tests with some flag and navigating > to the test page? I've also added a few more tracing markers in patchset #6 > here, hopefully that will allow us to get a better understanding what is taking > so much time during appendStream test. The first order of business would be to get the test running under an direct invocation of content shell and not via run-webkit-tests, so that you can actually poke around. That should be a matter of using --run-layout-test and I guess since this test needs a server you'll have to poke around a bit. I'd start by just determining if this is CPU bound or not, and I'd be kind of surprised if it is. Assuming it isn't I guess insert timestamped log statements in the codepaths this test will take over and over and sprinkle them in wider circles until you can see between which two "steps" the delay is. Good bets would be hitting the network (perhaps caching doesn't work or is defeated) or a timeout hack somewhere.
Took another look at the code, but haven't looked at the test this time. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... File Source/modules/mediasource/MediaSource.h (right): https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/MediaSource.h:104: HTMLMediaElement* getHTMLMediaElement() const; Getters don't often has "get" in Blink, just mediaElement() would be in line with some other APIs. Also, if this can never be null, have it return a reference instead. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:522: // 3.5.4 Prepare Append Algorithm Section numbers tend to change, but they are consistently used elsewhere in modules/mediasource/ (didn't check) and perhaps even correct elsewhere, then OK :) https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:523: // 1. If the SourceBuffer has been removed from the sourceBuffers attribute of the parent media source then throw an InvalidStateError exception and abort these steps. This is indented more than (some of) the steps that follow. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:530: // 3. If the HTMLMediaElement.error attribute is not null. then throw an InvalidStateError exception and abort these steps. https://www.w3.org/Bugs/Public/show_bug.cgi?id=28881 https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:532: exceptionState.throwDOMException(InvalidStateError, "Error detected in HTMLMediaElement this SourceBuffer is attached to."); This error message doesn't appear in any test expectations, so I guess this is untested? I'd perhaps make it more boring by saying just "The HTMLMediaElement.error attribute is not null." https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:539: // 2. Queue a task to fire a simple event named sourceopen at the parent media source . https://www.w3.org/Bugs/Public/show_bug.cgi?id=28882 https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:556: double currentTime = m_source->getHTMLMediaElement()->currentTime(); Can m_source or m_source->getHTMLMediaElement() be null? https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:764: // 1. Run the reset parser state algorithm. (Handled by caller) Is there anything that can be asserted to check that the caller did what it should? https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:776: if (decodeError) { Don't need the {} for one line. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:804: // 11. If the buffer full flag equals true, then run the append error algorithm with the decode error parameter set to false and abort this algorithm. This and some other comments is a bit long, if you can find some rough max line length in this file and keep comments to that (probably 80, 100 or 120 columns) that would make it more readable. https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm Playback position in HTMLMediaElement is unfortunately a bit of a mess. Can you check which of these two concepts actually makes sense in this context? https://html.spec.whatwg.org/#current-playback-position https://html.spec.whatwg.org/#official-playback-position We don't have them accessible by their per-spec names in Blink, but if it's the real playback position that's needed here and not one that compensates for seeking, then HTMLMediaElement::currentTime() isn't the right thing to use, instead one has to go to WebMediaPlayer::currentTime() directly. I'd also name the variable to match, probably currentPlaybackPosition(). If you want to expose HTMLMediaElement::currentPlaybackPosition() in the process, that'd be nice.
https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... File Source/modules/mediasource/MediaSource.h (right): https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/MediaSource.h:104: HTMLMediaElement* getHTMLMediaElement() const; On 2015/07/02 09:48:21, philipj wrote: > Getters don't often has "get" in Blink, just mediaElement() would be in line > with some other APIs. Also, if this can never be null, have it return a > reference instead. Done. I've renamed to just mediaElement, but I've kept the signature, since mediaElement() may return null when MediaSource is in closed state (see MediaSource::setReadyState). https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:522: // 3.5.4 Prepare Append Algorithm On 2015/07/02 09:48:21, philipj wrote: > Section numbers tend to change, but they are consistently used elsewhere in > modules/mediasource/ (didn't check) and perhaps even correct elsewhere, then OK > :) Acknowledged. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:523: // 1. If the SourceBuffer has been removed from the sourceBuffers attribute of the parent media source then throw an InvalidStateError exception and abort these steps. On 2015/07/02 09:48:21, philipj wrote: > This is indented more than (some of) the steps that follow. Done. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:530: // 3. If the HTMLMediaElement.error attribute is not null. then throw an InvalidStateError exception and abort these steps. On 2015/07/02 09:48:21, philipj wrote: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28881 Done. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:532: exceptionState.throwDOMException(InvalidStateError, "Error detected in HTMLMediaElement this SourceBuffer is attached to."); On 2015/07/02 09:48:21, philipj wrote: > This error message doesn't appear in any test expectations, so I guess this is > untested? I'd perhaps make it more boring by saying just "The > HTMLMediaElement.error attribute is not null." Done. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:539: // 2. Queue a task to fire a simple event named sourceopen at the parent media source . On 2015/07/02 09:48:21, philipj wrote: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28882 Done. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:556: double currentTime = m_source->getHTMLMediaElement()->currentTime(); On 2015/07/02 09:48:21, philipj wrote: > Can m_source or m_source->getHTMLMediaElement() be null? It shouldn't be null here, since evictCodedFrame is an internal method that is called only from appendBuffer/appendStream, and we should error out sooner (via throwExceptionIfRemovedOrUpdating) if we are not in the opened state. So I'll just add asserts here. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:764: // 1. Run the reset parser state algorithm. (Handled by caller) On 2015/07/02 09:48:21, philipj wrote: > Is there anything that can be asserted to check that the caller did what it > should? TBH, initially I just copied this block of code from appendBufferInternal/appendStreamInternal. But I guess we could call m_webSourceBuffer->abort() here, that should ensure that parser state is reset (see step #4 of SourceBuffer::about here). https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:776: if (decodeError) { On 2015/07/02 09:48:22, philipj wrote: > Don't need the {} for one line. Done. https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:804: // 11. If the buffer full flag equals true, then run the append error algorithm with the decode error parameter set to false and abort this algorithm. On 2015/07/02 09:48:21, philipj wrote: > This and some other comments is a bit long, if you can find some rough max line > length in this file and keep comments to that (probably 80, 100 or 120 columns) > that would make it more readable. I was just trying to be consistent with the rest of the code in this file. I've noticed that comments and even code in other methods don't seem to have any line length limits (e.g. take a look at SourceBuffer::remove in this file, line 336 is 265 chars long, and that code, not even something like comment with URL, which can't be wrapped). So should I reformat the whole file or keep it the old way? https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/02 09:48:22, philipj wrote: > Playback position in HTMLMediaElement is unfortunately a bit of a mess. Can you > check which of these two concepts actually makes sense in this context? > https://html.spec.whatwg.org/#current-playback-position > https://html.spec.whatwg.org/#official-playback-position > > We don't have them accessible by their per-spec names in Blink, but if it's the > real playback position that's needed here and not one that compensates for > seeking, then HTMLMediaElement::currentTime() isn't the right thing to use, > instead one has to go to WebMediaPlayer::currentTime() directly. > > I'd also name the variable to match, probably currentPlaybackPosition(). If you > want to expose HTMLMediaElement::currentPlaybackPosition() in the process, > that'd be nice. Actually we do want the position to reflect the pending seek position as soon as seeking starts, so I believe HTMLMediaElement::currentTime is the best choice here. Think about it - we are going to use this value to decide which data to keep during MSE GC, and if a seek operation has been started, then we should try to keep data appended at the new playback position/seek target, rather than the old playback position. This should fix crbug.com/440173 (see the comments there).
On 2015/07/02 09:27:46, philipj wrote: > On 2015/06/29 23:49:07, servolk wrote: > > On 2015/06/29 07:29:37, philipj wrote: > > > Sorry, this has been sitting idle for a while. Is it now waiting for final > > > review, or are you investigating why the test is so slow? > > > > I've made some changes to both this CL and related Chromium CL > > https://codereview.chromium.org/1008463002/ to pass current playback position > > down from blink level into the evictCodedFrames algorithm on > SourceBufferStream > > level and I was waiting for wolenetz@ to take a look at those changes. In the > > meantime I've also made some adjustments to this CL (patchsets 6-8): > > Moved "prepare append algorithm" into a separate prepareAppend method to > remove > > some code duplicated between appendStreamInternal and appendBufferInternal. > Then > > in PS 7-8 I've also added MediaSource::getHTMLMediaElement - which returns > > HTMLMediaElement that the MediaSource is attached to. This will allow us to > > obtain HTMLMediaElement::currentTime, which is going to be used for GC > > algorithm, and also will allow us to check HTMLMediaElement.error property > value > > in the step 3 of prepareAppend algorithm as described in MSE spec. So I guess > it > > would make sense for you also to take one more look at this blink CL. > > Re performance - I still don't know why the test is so slow. I was hoping to > > make it fast by limiting the size of the SourceBuffer (CL > > https://codereview.chromium.org/1182183004/) but ddorwin@ objected to that. So > > now I will either need a better approach to set SourceBuffer memory limits, or > > try to do some profiling of the test. Unfortunately I'm not very familiar with > > blink, can you recommend a way to analyze performance of a layout test? Is > there > > a way to run layout test with profiling and/or tracing turned on? Or is there > a > > way to run a layout test by starting blink_tests with some flag and navigating > > to the test page? I've also added a few more tracing markers in patchset #6 > > here, hopefully that will allow us to get a better understanding what is > taking > > so much time during appendStream test. > > The first order of business would be to get the test running under an direct > invocation of content shell and not via run-webkit-tests, so that you can > actually poke around. That should be a matter of using --run-layout-test and I > guess since this test needs a server you'll have to poke around a bit. > > I'd start by just determining if this is CPU bound or not, and I'd be kind of > surprised if it is. Assuming it isn't I guess insert timestamped log statements > in the codepaths this test will take over and over and sprinkle them in wider > circles until you can see between which two "steps" the delay is. Good bets > would be hitting the network (perhaps caching doesn't work or is defeated) or a > timeout hack somewhere. Regarding the test performance, the test does seem to be CPU bound (content_shell is using 100% of CPU according to top while the test is running). I've profiled the test with perf and it looks like most of the time is spent in networking stack. It makes sense - since the input test stream is very short (~10Kb) and we need to get 12Mb of data to trigger GC, it means we have to do ~1200 XHR requests, which incurs some overhead (even though request data is read from cache). I've tried replacing the short ~10Kb input stream with longer ~2Mb stream, and that reduced the total combined run time of the two new tests from ~17 seconds to under 5 seconds.
On 2015/07/08 01:18:40, servolk wrote: > Regarding the test performance, the test does seem to be CPU bound > (content_shell is using 100% of CPU according to top while the test is running). > I've profiled the test with perf and it looks like most of the time is spent in > networking stack. It makes sense - since the input test stream is very short > (~10Kb) and we need to get 12Mb of data to trigger GC, it means we have to do > ~1200 XHR requests, which incurs some overhead (even though request data is read > from cache). I've tried replacing the short ~10Kb input stream with longer ~2Mb > stream, and that reduced the total combined run time of the two new tests from > ~17 seconds to under 5 seconds. That sounds good enough! Can you use an existing test file, or will a new 2MB file be needed in the repo? Also, even with a 10KB input file, shouldn't it be enough to XHR it once, and simply call appendBuffer() many times with the same input? Or does that neuter the array? If it does, perhaps just copy it, that should still be much faster than hitting the network.
https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:804: // 11. If the buffer full flag equals true, then run the append error algorithm with the decode error parameter set to false and abort this algorithm. On 2015/07/07 23:01:52, servolk wrote: > On 2015/07/02 09:48:21, philipj wrote: > > This and some other comments is a bit long, if you can find some rough max > line > > length in this file and keep comments to that (probably 80, 100 or 120 > columns) > > that would make it more readable. > > I was just trying to be consistent with the rest of the code in this file. I've > noticed that comments and even code in other methods don't seem to have any line > length limits (e.g. take a look at SourceBuffer::remove in this file, line 336 > is 265 chars long, and that code, not even something like comment with URL, > which can't be wrapped). So should I reformat the whole file or keep it the old > way? Looks like different parts of this file use different wrapping widths for comments, but no limit for code. No need to do anything, everything is a mess :) https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/07 23:01:52, servolk wrote: > On 2015/07/02 09:48:22, philipj wrote: > > Playback position in HTMLMediaElement is unfortunately a bit of a mess. Can > you > > check which of these two concepts actually makes sense in this context? > > https://html.spec.whatwg.org/#current-playback-position > > https://html.spec.whatwg.org/#official-playback-position > > > > We don't have them accessible by their per-spec names in Blink, but if it's > the > > real playback position that's needed here and not one that compensates for > > seeking, then HTMLMediaElement::currentTime() isn't the right thing to use, > > instead one has to go to WebMediaPlayer::currentTime() directly. > > > > I'd also name the variable to match, probably currentPlaybackPosition(). If > you > > want to expose HTMLMediaElement::currentPlaybackPosition() in the process, > > that'd be nice. > > Actually we do want the position to reflect the pending seek position as soon as > seeking starts, so I believe HTMLMediaElement::currentTime is the best choice > here. Think about it - we are going to use this value to decide which data to > keep during MSE GC, and if a seek operation has been started, then we should try > to keep data appended at the new playback position/seek target, rather than the > old playback position. This should fix crbug.com/440173 (see the comments > there). OK, so it sounds like the MSE spec actually says the wrong thing here? If you agree, can you file a spec bug to use "official playback position" instead of "current playback position"? https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/MediaSource.cpp:522: return m_attachedElement; The getters inlined in the header have .get(), can you check if this change will build with Oilpan without it? https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:44: #include "core/html/MediaError.h" Is this include needed, or is it enough to forward declare MediaError, given that it's not actually used? https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:767: // Blank line instead? https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:769: m_webSourceBuffer->abort(); The web-facing SourceBuffer.abort() does more than just run the reset parser state algorithm, per spec at least. Might this have other side effects, or is the naming just a bit confusing?
On 2015/07/08 12:31:55, philipj wrote: > On 2015/07/08 01:18:40, servolk wrote: > > Regarding the test performance, the test does seem to be CPU bound > > (content_shell is using 100% of CPU according to top while the test is > running). > > I've profiled the test with perf and it looks like most of the time is spent > in > > networking stack. It makes sense - since the input test stream is very short > > (~10Kb) and we need to get 12Mb of data to trigger GC, it means we have to do > > ~1200 XHR requests, which incurs some overhead (even though request data is > read > > from cache). I've tried replacing the short ~10Kb input stream with longer > ~2Mb > > stream, and that reduced the total combined run time of the two new tests from > > ~17 seconds to under 5 seconds. > > That sounds good enough! Can you use an existing test file, or will a new 2MB > file be needed in the repo? > > Also, even with a 10KB input file, shouldn't it be enough to XHR it once, and > simply call appendBuffer() many times with the same input? Or does that neuter > the array? If it does, perhaps just copy it, that should still be much faster > than hitting the network. Yes, I tried that at first (single XHR + multiple appendStream) - but that didn't work, because appendStream neuters the input stream. Also xhr.responseType is 'legacystream' (I tried also 'stream' and 'blob', but that didn't work), and I don't see a way to copy/clone that. I guess I can add a new 2Mb data file, because currently the largest available audio-only test data file is LayoutTests/http/tests/media/resources/media-source/webm/test-a-192k-44100Hz-1ch.webm which is 10Kb (there are some other larger audio+video files, but we want to use audio-only for this test, since audio SourceBuffer has the smallest default size limit of 12Mb).
https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/140001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:804: // 11. If the buffer full flag equals true, then run the append error algorithm with the decode error parameter set to false and abort this algorithm. On 2015/07/08 12:47:43, philipj wrote: > On 2015/07/07 23:01:52, servolk wrote: > > On 2015/07/02 09:48:21, philipj wrote: > > > This and some other comments is a bit long, if you can find some rough max > > line > > > length in this file and keep comments to that (probably 80, 100 or 120 > > columns) > > > that would make it more readable. > > > > I was just trying to be consistent with the rest of the code in this file. > I've > > noticed that comments and even code in other methods don't seem to have any > line > > length limits (e.g. take a look at SourceBuffer::remove in this file, line 336 > > is 265 chars long, and that code, not even something like comment with URL, > > which can't be wrapped). So should I reformat the whole file or keep it the > old > > way? > > Looks like different parts of this file use different wrapping widths for > comments, but no limit for code. No need to do anything, everything is a mess :) Acknowledged. https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/08 12:47:44, philipj wrote: > On 2015/07/07 23:01:52, servolk wrote: > > On 2015/07/02 09:48:22, philipj wrote: > > > Playback position in HTMLMediaElement is unfortunately a bit of a mess. Can > > you > > > check which of these two concepts actually makes sense in this context? > > > https://html.spec.whatwg.org/#current-playback-position > > > https://html.spec.whatwg.org/#official-playback-position > > > > > > We don't have them accessible by their per-spec names in Blink, but if it's > > the > > > real playback position that's needed here and not one that compensates for > > > seeking, then HTMLMediaElement::currentTime() isn't the right thing to use, > > > instead one has to go to WebMediaPlayer::currentTime() directly. > > > > > > I'd also name the variable to match, probably currentPlaybackPosition(). If > > you > > > want to expose HTMLMediaElement::currentPlaybackPosition() in the process, > > > that'd be nice. > > > > Actually we do want the position to reflect the pending seek position as soon > as > > seeking starts, so I believe HTMLMediaElement::currentTime is the best choice > > here. Think about it - we are going to use this value to decide which data to > > keep during MSE GC, and if a seek operation has been started, then we should > try > > to keep data appended at the new playback position/seek target, rather than > the > > old playback position. This should fix crbug.com/440173 (see the comments > > there). > > OK, so it sounds like the MSE spec actually says the wrong thing here? If you > agree, can you file a spec bug to use "official playback position" instead of > "current playback position"? Well, AFAIK MSE spec actually does not explicitly describe the garbage collection algorithm and leaves it up to implementation, so it also doesn't require that some specific 'current playback position' or 'official playback position' should be used here. The 'current playback position' is mentioned multiple times in MSE spec section 2.4.4 https://w3c.github.io/media-source/#buffer-monitoring, and we want our GC algorithm to preserve data around the 'current playback position', instead of internal demuxer read position. So I don't think there's any problems with MSE spec. Btw, as far as I can see the definitions of 'official' and 'current' positions in your links don't explicitly specify that the two are different during seeking. The definitions just say that the 'current' position is some position on the media timeline and the 'official' one is an approximation of 'current' which "is kept stable while scripts are running". But I'm not sure what does 'kept stable' means in this context. Does that mean that the 'official' position is kept at the old playback time until pending seek completes? That's not mentioned in the definition. https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/MediaSource.cpp:522: return m_attachedElement; On 2015/07/08 12:47:44, philipj wrote: > The getters inlined in the header have .get(), can you check if this change will > build with Oilpan without it? You are right. Changed to .get() for consistency with other getters. https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:44: #include "core/html/MediaError.h" On 2015/07/08 12:47:44, philipj wrote: > Is this include needed, or is it enough to forward declare MediaError, given > that it's not actually used? This is needed, we can't get away with just a forward declaration here, because HTMLMediaElement::error returns a smart pointer to MediaError, which will try to destroy MediaError object if it's not null, and for the object to be properly destroyed, we need to know it's size and full definition. https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:767: // On 2015/07/08 12:47:44, philipj wrote: > Blank line instead? Done. https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:769: m_webSourceBuffer->abort(); On 2015/07/08 12:47:44, philipj wrote: > The web-facing SourceBuffer.abort() does more than just run the reset parser > state algorithm, per spec at least. Might this have other side effects, or is > the naming just a bit confusing? Yes, this is a bit confusing. Note that I'm not calling SourceBuffer.abort(), I'm calling WebSourceBuffer::abort(), which is one of the steps of SourceBuffer::abort (see step #4 there and note the comment). I've looked at WebSourceBufferImpl::abort and ChunkDemuxer::Abort + associated code path and I believe this is the right thing to do here. Keep in mind that we are doing this from appendError, i.e. we are already in error state here, calling WebSourceBuffer::abort should ensure we fully flush/reset parser state (as a side effect a pending seek operation might complete, but I think that should be ok since we are about to report error, I don't think anybody expects us to remain in pending seek state after an error is reported).
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:48: xhr.responseType = 'legacystream'; Is there a reason you are using a stream instead of arraybuffer? That shouldn't have the problems of copying and appending a buffer repeatedly. https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:69: sourceBuffer.appendStream(xhr.response); Is there a reason you are using appendStream()? This is experimental [1] and will not ever ship as-is because the spec is changing [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27239
https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:48: xhr.responseType = 'legacystream'; On 2015/07/08 19:04:51, ddorwin wrote: > Is there a reason you are using a stream instead of arraybuffer? > > That shouldn't have the problems of copying and appending a buffer repeatedly. SourceBuffer::appendStream expects blink::Stream as an input parameter. As far as I can see the only way we would obtain that from XHR.response is to use 'legacystream' response type. https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:69: sourceBuffer.appendStream(xhr.response); On 2015/07/08 19:04:51, ddorwin wrote: > Is there a reason you are using appendStream()? This is experimental [1] and > will not ever ship as-is because the spec is changing [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > [2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27239 Philip asked me to add a test case for appendStream (see https://codereview.chromium.org/1013923002/#msg4), because we might throw QuotaExceededError on two different code paths - appendBuffer and appendStream. So there are two new test cases here - one for appendBuffer (see above) and this one for appendStream.
https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:48: xhr.responseType = 'legacystream'; On 2015/07/08 19:13:00, servolk wrote: > On 2015/07/08 19:04:51, ddorwin wrote: > > Is there a reason you are using a stream instead of arraybuffer? > > > > That shouldn't have the problems of copying and appending a buffer repeatedly. > > SourceBuffer::appendStream expects blink::Stream as an input parameter. As far > as I can see the only way we would obtain that from XHR.response is to use > 'legacystream' response type. Can you instead use a random stream? (Is there any demuxing going on in this test?) For example, see https://github.com/dominictarr/stream-tester#createrandomstream-generator-max. Even if it can't be random, you should be able to create a stream that concatenates the |mediaData| without doing an XHR. However, I don't know whether the same is possible for this version of appendStream(), which may not be compatible with the current Streams spec. If it's using blink::Stream, though, it seems like it should work. https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:69: sourceBuffer.appendStream(xhr.response); On 2015/07/08 19:13:00, servolk wrote: > On 2015/07/08 19:04:51, ddorwin wrote: > > Is there a reason you are using appendStream()? This is experimental [1] and > > will not ever ship as-is because the spec is changing [2]. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > [2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27239 > > Philip asked me to add a test case for appendStream (see > https://codereview.chromium.org/1013923002/#msg4), because we might throw > QuotaExceededError on two different code paths - appendBuffer and appendStream. > So there are two new test cases here - one for appendBuffer (see above) and this > one for appendStream. I see, you're testing both, but only this one uses XHR. I think there should generally be only one test per file (W3C style). You would then have the method in the filename. If the suggestion above won't work, I'm not sure this is worth solving at this point and it may make more sense to just file a bug to add this test when appendStream() is updated.
https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:48: xhr.responseType = 'legacystream'; On 2015/07/08 19:24:09, ddorwin wrote: > On 2015/07/08 19:13:00, servolk wrote: > > On 2015/07/08 19:04:51, ddorwin wrote: > > > Is there a reason you are using a stream instead of arraybuffer? > > > > > > That shouldn't have the problems of copying and appending a buffer > repeatedly. > > > > SourceBuffer::appendStream expects blink::Stream as an input parameter. As far > > as I can see the only way we would obtain that from XHR.response is to use > > 'legacystream' response type. > > Can you instead use a random stream? (Is there any demuxing going on in this > test?) For example, see > https://github.com/dominictarr/stream-tester#createrandomstream-generator-max. > Even if it can't be random, you should be able to create a stream that > concatenates the |mediaData| without doing an XHR. > > However, I don't know whether the same is possible for this version of > appendStream(), which may not be compatible with the current Streams spec. If > it's using blink::Stream, though, it seems like it should work. No, it can't be random data, I tried that already. As soon as we append data to SourceBuffer it gets passed to the stream parser, which throws errors if data is invalid and that prevents the test from working as intended. We need valid data here. I've been also thinking about generating it somehow, but generating a valid webm bitstream from Javascript seems to be non-trivial to do. We had a discussion with Philip about this issue in a separate email thread (I can forward you if you are interested). And so far appending the same stream in 'sequence' mode seems to be the easiest solution. We could probably try downloading the input stream once, then extracting all data from it, then generating new stream objects from that data, but with the new test 2Mb stream that I've added the test runtime is now under 3 seconds and it's probably not worth additional effort to do that now. https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:69: sourceBuffer.appendStream(xhr.response); On 2015/07/08 19:24:09, ddorwin wrote: > On 2015/07/08 19:13:00, servolk wrote: > > On 2015/07/08 19:04:51, ddorwin wrote: > > > Is there a reason you are using appendStream()? This is experimental [1] and > > > will not ever ship as-is because the spec is changing [2]. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > [2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27239 > > > > Philip asked me to add a test case for appendStream (see > > https://codereview.chromium.org/1013923002/#msg4), because we might throw > > QuotaExceededError on two different code paths - appendBuffer and > appendStream. > > So there are two new test cases here - one for appendBuffer (see above) and > this > > one for appendStream. > > I see, you're testing both, but only this one uses XHR. > I think there should generally be only one test per file (W3C style). You would > then have the method in the filename. Hmm, that's definitely not the case currently - look at the existing tests in chromium/src/third_party/WebKit/LayoutTests/http/tests/media/media-source/ - most of the files have multiple test cases in them, grouped by the functionality being tested. So I did the same. > If the suggestion above won't work, I'm not sure this is worth solving at this > point and it may make more sense to just file a bug to add this test when > appendStream() is updated. I'd be ok with that if Philip agrees (after all he's listed as the sole owner in third_party/WebKit/Source/modules/mediasource/OWNERS). But on the other hand, now that I have added a larger test file, the total run time of these two new test cases is ~3 seconds combined, so we can as well keep them now.
https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/08 19:03:47, servolk wrote: > On 2015/07/08 12:47:44, philipj wrote: > > On 2015/07/07 23:01:52, servolk wrote: > > > On 2015/07/02 09:48:22, philipj wrote: > > > > Playback position in HTMLMediaElement is unfortunately a bit of a mess. > Can > > > you > > > > check which of these two concepts actually makes sense in this context? > > > > https://html.spec.whatwg.org/#current-playback-position > > > > https://html.spec.whatwg.org/#official-playback-position > > > > > > > > We don't have them accessible by their per-spec names in Blink, but if > it's > > > the > > > > real playback position that's needed here and not one that compensates for > > > > seeking, then HTMLMediaElement::currentTime() isn't the right thing to > use, > > > > instead one has to go to WebMediaPlayer::currentTime() directly. > > > > > > > > I'd also name the variable to match, probably currentPlaybackPosition(). > If > > > you > > > > want to expose HTMLMediaElement::currentPlaybackPosition() in the process, > > > > that'd be nice. > > > > > > Actually we do want the position to reflect the pending seek position as > soon > > as > > > seeking starts, so I believe HTMLMediaElement::currentTime is the best > choice > > > here. Think about it - we are going to use this value to decide which data > to > > > keep during MSE GC, and if a seek operation has been started, then we should > > try > > > to keep data appended at the new playback position/seek target, rather than > > the > > > old playback position. This should fix crbug.com/440173 (see the comments > > > there). > > > > OK, so it sounds like the MSE spec actually says the wrong thing here? If you > > agree, can you file a spec bug to use "official playback position" instead of > > "current playback position"? > > Well, AFAIK MSE spec actually does not explicitly describe the garbage > collection algorithm and leaves it up to implementation, so it also doesn't > require that some specific 'current playback position' or 'official playback > position' should be used here. The 'current playback position' is mentioned > multiple times in MSE spec section 2.4.4 > https://w3c.github.io/media-source/#buffer-monitoring, and we want our GC > algorithm to preserve data around the 'current playback position', instead of > internal demuxer read position. So I don't think there's any problems with MSE > spec. > Btw, as far as I can see the definitions of 'official' and 'current' positions > in your links don't explicitly specify that the two are different during > seeking. The definitions just say that the 'current' position is some position > on the media timeline and the 'official' one is an approximation of 'current' > which "is kept stable while scripts are running". But I'm not sure what does > 'kept stable' means in this context. Does that mean that the 'official' position > is kept at the old playback time until pending seek completes? That's not > mentioned in the definition. One instance of "current playback position" is linked to http://www.w3.org/TR/html5/embedded-content-0.html#current-playback-position, which is the same as the WHATWG spec in this case. The definition of "official playback position" just says that it's initially zero, there are other parts of the spec where it's set to various values, notably "Any time the user agent provides a stable state, the official playback position must be set to the current playback position." However, you'll notice some discrepancies around seeking when comparing what the spec says and how it's implemented. Per the spec, setting currentTime doesn't synchronously set it to the new value, the seeking algorithm returns early and only later would make the change visible in currentTime. Blink, however, has an if (m_seeking) return m_lastSeekTime bit in the currentTime setter. So it's a bit of a mess, and I've filed a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28929 In the meantime, I guess we just need to decide how this should work, implement that with a sprinkling of TODOs, and keep track of the spec bugs for the mismatch. Out of "current playback position", "official playback position", "default playback start position", a non-spec'd "seek position" and the combinations of those that is currentTime, what's the right time to use? https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:44: #include "core/html/MediaError.h" On 2015/07/08 19:03:48, servolk wrote: > On 2015/07/08 12:47:44, philipj wrote: > > Is this include needed, or is it enough to forward declare MediaError, given > > that it's not actually used? > > This is needed, we can't get away with just a forward declaration here, because > HTMLMediaElement::error returns a smart pointer to MediaError, which will try to > destroy MediaError object if it's not null, and for the object to be properly > destroyed, we need to know it's size and full definition. Acknowledged. https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:769: m_webSourceBuffer->abort(); On 2015/07/08 19:03:47, servolk wrote: > On 2015/07/08 12:47:44, philipj wrote: > > The web-facing SourceBuffer.abort() does more than just run the reset parser > > state algorithm, per spec at least. Might this have other side effects, or is > > the naming just a bit confusing? > > Yes, this is a bit confusing. Note that I'm not calling SourceBuffer.abort(), > I'm calling WebSourceBuffer::abort(), which is one of the steps of > SourceBuffer::abort (see step #4 there and note the comment). I've looked at > WebSourceBufferImpl::abort and ChunkDemuxer::Abort + associated code path and I > believe this is the right thing to do here. Keep in mind that we are doing this > from appendError, i.e. we are already in error state here, calling > WebSourceBuffer::abort should ensure we fully flush/reset parser state (as a > side effect a pending seek operation might complete, but I think that should be > ok since we are about to report error, I don't think anybody expects us to > remain in pending seek state after an error is reported). Understood. Do you think resetParserState() would be a more appropriate name than abort() on WebSourceBuffer?
https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html (right): https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:69: sourceBuffer.appendStream(xhr.response); On 2015/07/08 20:58:26, servolk wrote: > On 2015/07/08 19:24:09, ddorwin wrote: > > I see, you're testing both, but only this one uses XHR. > > I think there should generally be only one test per file (W3C style). You > would > > then have the method in the filename. > > Hmm, that's definitely not the case currently - look at the existing tests in > chromium/src/third_party/WebKit/LayoutTests/http/tests/media/media-source/ - > most of the files have multiple test cases in them, grouped by the functionality > being tested. So I did the same. > > > If the suggestion above won't work, I'm not sure this is worth solving at this > > point and it may make more sense to just file a bug to add this test when > > appendStream() is updated. > > I'd be ok with that if Philip agrees (after all he's listed as the sole owner in > third_party/WebKit/Source/modules/mediasource/OWNERS). But on the other hand, > now that I have added a larger test file, the total run time of these two new > test cases is ~3 seconds combined, so we can as well keep them now. I've certainly written many web-platform-tests that combine many small, related tests in a single file, when doing so allowed test code to be shared and splitting them into a script + test files didn't seem like an improvement. However, in complex tests that are more likely to be flaky or slow, having them together can make the problem worse, and if one fails it's harder to figure out which one it was. So this is a case where splitting them might make sense.
On 2015/07/09 09:56:05, philipj wrote: > https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... > File LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html > (right): > > https://codereview.chromium.org/1013923002/diff/160001/LayoutTests/http/tests... > LayoutTests/http/tests/media/media-source/mediasource-evict-frames.html:69: > sourceBuffer.appendStream(xhr.response); > On 2015/07/08 20:58:26, servolk wrote: > > On 2015/07/08 19:24:09, ddorwin wrote: > > > I see, you're testing both, but only this one uses XHR. > > > I think there should generally be only one test per file (W3C style). You > > would > > > then have the method in the filename. > > > > Hmm, that's definitely not the case currently - look at the existing tests in > > chromium/src/third_party/WebKit/LayoutTests/http/tests/media/media-source/ - > > most of the files have multiple test cases in them, grouped by the > functionality > > being tested. So I did the same. > > > > > If the suggestion above won't work, I'm not sure this is worth solving at > this > > > point and it may make more sense to just file a bug to add this test when > > > appendStream() is updated. > > > > I'd be ok with that if Philip agrees (after all he's listed as the sole owner > in > > third_party/WebKit/Source/modules/mediasource/OWNERS). But on the other hand, > > now that I have added a larger test file, the total run time of these two new > > test cases is ~3 seconds combined, so we can as well keep them now. > > I've certainly written many web-platform-tests that combine many small, related > tests in a single file, when doing so allowed test code to be shared and > splitting them into a script + test files didn't seem like an improvement. > > However, in complex tests that are more likely to be flaky or slow, having them > together can make the problem worse, and if one fails it's harder to figure out > which one it was. > > So this is a case where splitting them might make sense. Ok, I've split the appendBuffer/appendSegment tests into two separate files. PTAL. Btw, I've also made a change to pass the new data size, about to be appended, into the evictCodedFrames algorithm. This should be useful for figuring out how much data needs to be evicted on ChunkDemuxer side.
https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/09 09:49:23, philipj wrote: > On 2015/07/08 19:03:47, servolk wrote: > > On 2015/07/08 12:47:44, philipj wrote: > > > On 2015/07/07 23:01:52, servolk wrote: > > > > On 2015/07/02 09:48:22, philipj wrote: > > > > > Playback position in HTMLMediaElement is unfortunately a bit of a mess. > > Can > > > > you > > > > > check which of these two concepts actually makes sense in this context? > > > > > https://html.spec.whatwg.org/#current-playback-position > > > > > https://html.spec.whatwg.org/#official-playback-position > > > > > > > > > > We don't have them accessible by their per-spec names in Blink, but if > > it's > > > > the > > > > > real playback position that's needed here and not one that compensates > for > > > > > seeking, then HTMLMediaElement::currentTime() isn't the right thing to > > use, > > > > > instead one has to go to WebMediaPlayer::currentTime() directly. > > > > > > > > > > I'd also name the variable to match, probably currentPlaybackPosition(). > > If > > > > you > > > > > want to expose HTMLMediaElement::currentPlaybackPosition() in the > process, > > > > > that'd be nice. > > > > > > > > Actually we do want the position to reflect the pending seek position as > > soon > > > as > > > > seeking starts, so I believe HTMLMediaElement::currentTime is the best > > choice > > > > here. Think about it - we are going to use this value to decide which data > > to > > > > keep during MSE GC, and if a seek operation has been started, then we > should > > > try > > > > to keep data appended at the new playback position/seek target, rather > than > > > the > > > > old playback position. This should fix crbug.com/440173 (see the comments > > > > there). > > > > > > OK, so it sounds like the MSE spec actually says the wrong thing here? If > you > > > agree, can you file a spec bug to use "official playback position" instead > of > > > "current playback position"? > > > > Well, AFAIK MSE spec actually does not explicitly describe the garbage > > collection algorithm and leaves it up to implementation, so it also doesn't > > require that some specific 'current playback position' or 'official playback > > position' should be used here. The 'current playback position' is mentioned > > multiple times in MSE spec section 2.4.4 > > https://w3c.github.io/media-source/#buffer-monitoring, and we want our GC > > algorithm to preserve data around the 'current playback position', instead of > > internal demuxer read position. So I don't think there's any problems with MSE > > spec. > > Btw, as far as I can see the definitions of 'official' and 'current' positions > > in your links don't explicitly specify that the two are different during > > seeking. The definitions just say that the 'current' position is some position > > on the media timeline and the 'official' one is an approximation of 'current' > > which "is kept stable while scripts are running". But I'm not sure what does > > 'kept stable' means in this context. Does that mean that the 'official' > position > > is kept at the old playback time until pending seek completes? That's not > > mentioned in the definition. > > One instance of "current playback position" is linked to > http://www.w3.org/TR/html5/embedded-content-0.html#current-playback-position, > which is the same as the WHATWG spec in this case. > > The definition of "official playback position" just says that it's initially > zero, there are other parts of the spec where it's set to various values, > notably "Any time the user agent provides a stable state, the official playback > position must be set to the current playback position." > > However, you'll notice some discrepancies around seeking when comparing what the > spec says and how it's implemented. Per the spec, setting currentTime doesn't > synchronously set it to the new value, the seeking algorithm returns early and > only later would make the change visible in currentTime. Blink, however, has an > if (m_seeking) return m_lastSeekTime bit in the currentTime setter. So it's a > bit of a mess, and I've filed a spec bug: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28929 > > In the meantime, I guess we just need to decide how this should work, implement > that with a sprinkling of TODOs, and keep track of the spec bugs for the > mismatch. Out of "current playback position", "official playback position", > "default playback start position", a non-spec'd "seek position" and the > combinations of those that is currentTime, what's the right time to use? I believe HTMLMediaElement::currentTime is exactly what we want. It is current playback position for normal playback case, but it changes to the new seek position as soon as seeking starts and that's what we want to protect during GC (evictCodedFrames). After all this is sorted out, will there be some property on HTMLMediaElement with the same semantics as the current implementation of currentTime? https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:769: m_webSourceBuffer->abort(); On 2015/07/09 09:49:23, philipj wrote: > On 2015/07/08 19:03:47, servolk wrote: > > On 2015/07/08 12:47:44, philipj wrote: > > > The web-facing SourceBuffer.abort() does more than just run the reset parser > > > state algorithm, per spec at least. Might this have other side effects, or > is > > > the naming just a bit confusing? > > > > Yes, this is a bit confusing. Note that I'm not calling SourceBuffer.abort(), > > I'm calling WebSourceBuffer::abort(), which is one of the steps of > > SourceBuffer::abort (see step #4 there and note the comment). I've looked at > > WebSourceBufferImpl::abort and ChunkDemuxer::Abort + associated code path and > I > > believe this is the right thing to do here. Keep in mind that we are doing > this > > from appendError, i.e. we are already in error state here, calling > > WebSourceBuffer::abort should ensure we fully flush/reset parser state (as a > > side effect a pending seek operation might complete, but I think that should > be > > ok since we are about to report error, I don't think anybody expects us to > > remain in pending seek state after an error is reported). > > Understood. Do you think resetParserState() would be a more appropriate name > than abort() on WebSourceBuffer? Yes, I think it might a good idea to rename the current WebSourceBuffer::abort into WebSourceBuffer::resetParserState, but I'd like also wolenetz@ to chime in on this. I'm not sure if completing a pending seek operation in ChunkDemuxer can be decoupled from the parser reset algorithm.
https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/10 01:11:27, servolk wrote: > On 2015/07/09 09:49:23, philipj wrote: > > On 2015/07/08 19:03:47, servolk wrote: > > > On 2015/07/08 12:47:44, philipj wrote: > > > > On 2015/07/07 23:01:52, servolk wrote: > > > > > On 2015/07/02 09:48:22, philipj wrote: > > > > > > Playback position in HTMLMediaElement is unfortunately a bit of a > mess. > > > Can > > > > > you > > > > > > check which of these two concepts actually makes sense in this > context? > > > > > > https://html.spec.whatwg.org/#current-playback-position > > > > > > https://html.spec.whatwg.org/#official-playback-position > > > > > > > > > > > > We don't have them accessible by their per-spec names in Blink, but if > > > it's > > > > > the > > > > > > real playback position that's needed here and not one that compensates > > for > > > > > > seeking, then HTMLMediaElement::currentTime() isn't the right thing to > > > use, > > > > > > instead one has to go to WebMediaPlayer::currentTime() directly. > > > > > > > > > > > > I'd also name the variable to match, probably > currentPlaybackPosition(). > > > If > > > > > you > > > > > > want to expose HTMLMediaElement::currentPlaybackPosition() in the > > process, > > > > > > that'd be nice. > > > > > > > > > > Actually we do want the position to reflect the pending seek position as > > > soon > > > > as > > > > > seeking starts, so I believe HTMLMediaElement::currentTime is the best > > > choice > > > > > here. Think about it - we are going to use this value to decide which > data > > > to > > > > > keep during MSE GC, and if a seek operation has been started, then we > > should > > > > try > > > > > to keep data appended at the new playback position/seek target, rather > > than > > > > the > > > > > old playback position. This should fix crbug.com/440173 (see the > comments > > > > > there). > > > > > > > > OK, so it sounds like the MSE spec actually says the wrong thing here? If > > you > > > > agree, can you file a spec bug to use "official playback position" instead > > of > > > > "current playback position"? > > > > > > Well, AFAIK MSE spec actually does not explicitly describe the garbage > > > collection algorithm and leaves it up to implementation, so it also doesn't > > > require that some specific 'current playback position' or 'official playback > > > position' should be used here. The 'current playback position' is mentioned > > > multiple times in MSE spec section 2.4.4 > > > https://w3c.github.io/media-source/#buffer-monitoring, and we want our GC > > > algorithm to preserve data around the 'current playback position', instead > of > > > internal demuxer read position. So I don't think there's any problems with > MSE > > > spec. > > > Btw, as far as I can see the definitions of 'official' and 'current' > positions > > > in your links don't explicitly specify that the two are different during > > > seeking. The definitions just say that the 'current' position is some > position > > > on the media timeline and the 'official' one is an approximation of > 'current' > > > which "is kept stable while scripts are running". But I'm not sure what does > > > 'kept stable' means in this context. Does that mean that the 'official' > > position > > > is kept at the old playback time until pending seek completes? That's not > > > mentioned in the definition. > > > > One instance of "current playback position" is linked to > > http://www.w3.org/TR/html5/embedded-content-0.html#current-playback-position, > > which is the same as the WHATWG spec in this case. > > > > The definition of "official playback position" just says that it's initially > > zero, there are other parts of the spec where it's set to various values, > > notably "Any time the user agent provides a stable state, the official > playback > > position must be set to the current playback position." > > > > However, you'll notice some discrepancies around seeking when comparing what > the > > spec says and how it's implemented. Per the spec, setting currentTime doesn't > > synchronously set it to the new value, the seeking algorithm returns early and > > only later would make the change visible in currentTime. Blink, however, has > an > > if (m_seeking) return m_lastSeekTime bit in the currentTime setter. So it's a > > bit of a mess, and I've filed a spec bug: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28929 > > > > In the meantime, I guess we just need to decide how this should work, > implement > > that with a sprinkling of TODOs, and keep track of the spec bugs for the > > mismatch. Out of "current playback position", "official playback position", > > "default playback start position", a non-spec'd "seek position" and the > > combinations of those that is currentTime, what's the right time to use? > > I believe HTMLMediaElement::currentTime is exactly what we want. It is current > playback position for normal playback case, but it changes to the new seek > position as soon as seeking starts and that's what we want to protect during GC > (evictCodedFrames). After all this is sorted out, will there be some property on > HTMLMediaElement with the same semantics as the current implementation of > currentTime? If there isn't an internal concept for it, then simply having the MSE spec be defined in terms of currentTime would also work. wolenetz@, what do you think, and should I file an MSE spec bug to sort this out?
Split tests look good, BTW.
At long last, and with apologies for delay, my CR comments: (Also, what is your plan for landing these two CLs without breaking GC in the interim?) https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... File public/platform/WebSourceBuffer.h (right): https://codereview.chromium.org/1013923002/diff/140001/public/platform/WebSou... public/platform/WebSourceBuffer.h:57: // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm On 2015/07/10 08:40:27, philipj wrote: > On 2015/07/10 01:11:27, servolk wrote: > > On 2015/07/09 09:49:23, philipj wrote: > > > On 2015/07/08 19:03:47, servolk wrote: > > > > On 2015/07/08 12:47:44, philipj wrote: > > > > > On 2015/07/07 23:01:52, servolk wrote: > > > > > > On 2015/07/02 09:48:22, philipj wrote: > > > > > > > Playback position in HTMLMediaElement is unfortunately a bit of a > > mess. > > > > Can > > > > > > you > > > > > > > check which of these two concepts actually makes sense in this > > context? > > > > > > > https://html.spec.whatwg.org/#current-playback-position > > > > > > > https://html.spec.whatwg.org/#official-playback-position > > > > > > > > > > > > > > We don't have them accessible by their per-spec names in Blink, but > if > > > > it's > > > > > > the > > > > > > > real playback position that's needed here and not one that > compensates > > > for > > > > > > > seeking, then HTMLMediaElement::currentTime() isn't the right thing > to > > > > use, > > > > > > > instead one has to go to WebMediaPlayer::currentTime() directly. > > > > > > > > > > > > > > I'd also name the variable to match, probably > > currentPlaybackPosition(). > > > > If > > > > > > you > > > > > > > want to expose HTMLMediaElement::currentPlaybackPosition() in the > > > process, > > > > > > > that'd be nice. > > > > > > > > > > > > Actually we do want the position to reflect the pending seek position > as > > > > soon > > > > > as > > > > > > seeking starts, so I believe HTMLMediaElement::currentTime is the best > > > > choice > > > > > > here. Think about it - we are going to use this value to decide which > > data > > > > to > > > > > > keep during MSE GC, and if a seek operation has been started, then we > > > should > > > > > try > > > > > > to keep data appended at the new playback position/seek target, rather > > > than > > > > > the > > > > > > old playback position. This should fix crbug.com/440173 (see the > > comments > > > > > > there). > > > > > > > > > > OK, so it sounds like the MSE spec actually says the wrong thing here? > If > > > you > > > > > agree, can you file a spec bug to use "official playback position" > instead > > > of > > > > > "current playback position"? > > > > > > > > Well, AFAIK MSE spec actually does not explicitly describe the garbage > > > > collection algorithm and leaves it up to implementation, so it also > doesn't > > > > require that some specific 'current playback position' or 'official > playback > > > > position' should be used here. The 'current playback position' is > mentioned > > > > multiple times in MSE spec section 2.4.4 > > > > https://w3c.github.io/media-source/#buffer-monitoring, and we want our GC > > > > algorithm to preserve data around the 'current playback position', instead > > of > > > > internal demuxer read position. So I don't think there's any problems with > > MSE > > > > spec. > > > > Btw, as far as I can see the definitions of 'official' and 'current' > > positions > > > > in your links don't explicitly specify that the two are different during > > > > seeking. The definitions just say that the 'current' position is some > > position > > > > on the media timeline and the 'official' one is an approximation of > > 'current' > > > > which "is kept stable while scripts are running". But I'm not sure what > does > > > > 'kept stable' means in this context. Does that mean that the 'official' > > > position > > > > is kept at the old playback time until pending seek completes? That's not > > > > mentioned in the definition. > > > > > > One instance of "current playback position" is linked to > > > > http://www.w3.org/TR/html5/embedded-content-0.html#current-playback-position, > > > which is the same as the WHATWG spec in this case. > > > > > > The definition of "official playback position" just says that it's initially > > > zero, there are other parts of the spec where it's set to various values, > > > notably "Any time the user agent provides a stable state, the official > > playback > > > position must be set to the current playback position." > > > > > > However, you'll notice some discrepancies around seeking when comparing what > > the > > > spec says and how it's implemented. Per the spec, setting currentTime > doesn't > > > synchronously set it to the new value, the seeking algorithm returns early > and > > > only later would make the change visible in currentTime. Blink, however, has > > an > > > if (m_seeking) return m_lastSeekTime bit in the currentTime setter. So it's > a > > > bit of a mess, and I've filed a spec bug: > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28929 > > > > > > In the meantime, I guess we just need to decide how this should work, > > implement > > > that with a sprinkling of TODOs, and keep track of the spec bugs for the > > > mismatch. Out of "current playback position", "official playback position", > > > "default playback start position", a non-spec'd "seek position" and the > > > combinations of those that is currentTime, what's the right time to use? > > > > I believe HTMLMediaElement::currentTime is exactly what we want. It is current > > playback position for normal playback case, but it changes to the new seek > > position as soon as seeking starts and that's what we want to protect during > GC > > (evictCodedFrames). After all this is sorted out, will there be some property > on > > HTMLMediaElement with the same semantics as the current implementation of > > currentTime? > > If there isn't an internal concept for it, then simply having the MSE spec be > defined in terms of currentTime would also work. > > wolenetz@, what do you think, and should I file an MSE spec bug to sort this > out? Hmm. We need the current playback position, and if there is a seek, it needs to be the seek position as our implementation currently synchronously updates it (and as a reply on the WHATWG spec bug mentioned, seems sane.) However, as servolk@ mentioned, the MSE spec leaves selection of ranges for removal as part of GC up to implementations, and doesn't mention anything about current/official/seek playback position. In many other places in the MSE spec, we refer to current playback position (e.g. SourceBuffer monitoring). The intent to me appears to be similar: it should be like our current Blink HTMLMediaElement::currentTIme, which is updated synchronously on seek start. Since MSE extends the HTMLMediaElement, getting the latter's spec updated seems to me to be the right thing (especially since implementations already synchronously update currentTime). I'm less sure there needs to be any mention of this in the MSE spec. https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:769: m_webSourceBuffer->abort(); On 2015/07/10 01:11:28, servolk wrote: > On 2015/07/09 09:49:23, philipj wrote: > > On 2015/07/08 19:03:47, servolk wrote: > > > On 2015/07/08 12:47:44, philipj wrote: > > > > The web-facing SourceBuffer.abort() does more than just run the reset > parser > > > > state algorithm, per spec at least. Might this have other side effects, or > > is > > > > the naming just a bit confusing? > > > > > > Yes, this is a bit confusing. Note that I'm not calling > SourceBuffer.abort(), > > > I'm calling WebSourceBuffer::abort(), which is one of the steps of > > > SourceBuffer::abort (see step #4 there and note the comment). I've looked at > > > WebSourceBufferImpl::abort and ChunkDemuxer::Abort + associated code path > and > > I > > > believe this is the right thing to do here. Keep in mind that we are doing > > this > > > from appendError, i.e. we are already in error state here, calling > > > WebSourceBuffer::abort should ensure we fully flush/reset parser state (as a > > > side effect a pending seek operation might complete, but I think that should > > be > > > ok since we are about to report error, I don't think anybody expects us to > > > remain in pending seek state after an error is reported). > > > > Understood. Do you think resetParserState() would be a more appropriate name > > than abort() on WebSourceBuffer? > > Yes, I think it might a good idea to rename the current WebSourceBuffer::abort > into WebSourceBuffer::resetParserState, but I'd like also wolenetz@ to chime in > on this. > I'm not sure if completing a pending seek operation in ChunkDemuxer can be > decoupled from the parser reset algorithm. Rename to resetParserState of WebSourceBuffer::abort really sgtm. It's definitely not SourceBuffer::abort(), and the names being the same otherwise is really confusing. Renaming ChunkDemuxer::Abort() also sgtm. Resetting the parser state may indeed legally complete a pending seek, IIUC: it includes emitting any parsed but not-yet-processed coded frames, which may indeed allow seek to complete. https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:789: if (decodeError) This is interesting that Blink didn't do this previously. Internally in Chromium, we bubble the error through the pipeline / mediasourcedelegate, Do we need a new layout test added to confirm failed append does the right thing to the state of m_source? https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:813: bool evictCodedFramesResult = evictCodedFrames(0); This needs to be done, instead of here, within the loop, in didReceiveDataForClient, prior to each m_webSourceBUffer->append(...) in l.803. Also, that is the precise time at which you'll actually know a real dataLength. Is the intent of this implementation to try to free enough headroom prior to stream append loop, or during each iteration of the loop. I suggest the latter, though the Chromium side CL currently ignores newDataSize in WSBI::evictCodedFrames().
FYI: After discussing offline with wolenetz@ how we can land this safely without breaking anything in the intermediate states I've created a new CL (https://codereview.chromium.org/1309563003/) that introduces a dummy WebSourceBufferImpl::evictCodedFrames method that always returns true. I have also split the new LayoutTests previously introduced here into a separate CL (https://codereview.chromium.org/1304953002). So the landing plan is: 1. Land the Blink part of the CL first. It will call a dummy evictCodedFrames, but otherwise everything will be working the old way, modulo some refactoring. On the Chromium side evictCodedFrames will be a dummy and WebSourceBuffer::append will actually do the GC under the hood after appending the new data, just like it currently does. 2. Land the Chromium-side changes. That will replace WebSourceBufferImpl::evictCodedFrames with the actual implementation and will change the MSE GC logic on the Chromium side of things (+ that will also adjust Chromium-side unit tests). 3. Land the new LayoutTests (https://codereview.chromium.org/1304953002) - these should pass once the Chromium-side changes are in place.
https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/160001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:769: m_webSourceBuffer->abort(); On 2015/08/12 22:42:03, wolenetz wrote: > On 2015/07/10 01:11:28, servolk wrote: > > On 2015/07/09 09:49:23, philipj wrote: > > > On 2015/07/08 19:03:47, servolk wrote: > > > > On 2015/07/08 12:47:44, philipj wrote: > > > > > The web-facing SourceBuffer.abort() does more than just run the reset > > parser > > > > > state algorithm, per spec at least. Might this have other side effects, > or > > > is > > > > > the naming just a bit confusing? > > > > > > > > Yes, this is a bit confusing. Note that I'm not calling > > SourceBuffer.abort(), > > > > I'm calling WebSourceBuffer::abort(), which is one of the steps of > > > > SourceBuffer::abort (see step #4 there and note the comment). I've looked > at > > > > WebSourceBufferImpl::abort and ChunkDemuxer::Abort + associated code path > > and > > > I > > > > believe this is the right thing to do here. Keep in mind that we are doing > > > this > > > > from appendError, i.e. we are already in error state here, calling > > > > WebSourceBuffer::abort should ensure we fully flush/reset parser state (as > a > > > > side effect a pending seek operation might complete, but I think that > should > > > be > > > > ok since we are about to report error, I don't think anybody expects us to > > > > remain in pending seek state after an error is reported). > > > > > > Understood. Do you think resetParserState() would be a more appropriate name > > > than abort() on WebSourceBuffer? > > > > Yes, I think it might a good idea to rename the current WebSourceBuffer::abort > > into WebSourceBuffer::resetParserState, but I'd like also wolenetz@ to chime > in > > on this. > > I'm not sure if completing a pending seek operation in ChunkDemuxer can be > > decoupled from the parser reset algorithm. > > Rename to resetParserState of WebSourceBuffer::abort really sgtm. It's > definitely not SourceBuffer::abort(), and the names being the same otherwise is > really confusing. Renaming ChunkDemuxer::Abort() also sgtm. > > Resetting the parser state may indeed legally complete a pending seek, IIUC: it > includes emitting any parsed but not-yet-processed coded frames, which may > indeed allow seek to complete. Ok, I think renaming WebSourceBuffer::abort into resetParserState makes sense, but I'll do it as a separate CL after this one lands. https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:789: if (decodeError) On 2015/08/12 22:42:04, wolenetz wrote: > This is interesting that Blink didn't do this previously. Internally in > Chromium, we bubble the error through the pipeline / mediasourcedelegate, Do we > need a new layout test added to confirm failed append does the right thing to > the state of m_source? This is a new method that was introduced in this CL, so currently it is only going to be invoked from one place (didFinishLoading via appendStreamDone) and decodeError will always be false for now. But later we can re-route the actual decode errors, which are handled through a different code path, through SourceBuffer::appendError as well. https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:813: bool evictCodedFramesResult = evictCodedFrames(0); On 2015/08/12 22:42:04, wolenetz wrote: > This needs to be done, instead of here, within the loop, in > didReceiveDataForClient, prior to each m_webSourceBUffer->append(...) in l.803. > Also, that is the precise time at which you'll actually know a real dataLength. > > Is the intent of this implementation to try to free enough headroom prior to > stream append loop, or during each iteration of the loop. I suggest the latter, > though the Chromium side CL currently ignores newDataSize in > WSBI::evictCodedFrames(). Yeah, you are right, it's actually cleaner to do it in didReceiveDataForClient. We will only try to free enough headroom in advance if maxSize is specified for appendStream.
Recent changes (and thus the whole CL) LGTM.
Sending to CQ to improve chances of beating the branch point. Matt, if you had any further issues, I hope doing that in a follow-up is fine.
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1013923002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1013923002/330001
Message was sent while issue was closed.
Committed patchset #18 (id:330001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200989
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1013923002/diff/260001/Source/modules/mediaso... Source/modules/mediasource/SourceBuffer.cpp:789: if (decodeError) On 2015/08/21 02:51:25, servolk wrote: > On 2015/08/12 22:42:04, wolenetz wrote: > > This is interesting that Blink didn't do this previously. Internally in > > Chromium, we bubble the error through the pipeline / mediasourcedelegate, Do > we > > need a new layout test added to confirm failed append does the right thing to > > the state of m_source? > > This is a new method that was introduced in this CL, so currently it is only > going to be invoked from one place (didFinishLoading via appendStreamDone) and > decodeError will always be false for now. But later we can re-route the actual > decode errors, which are handled through a different code path, through > SourceBuffer::appendError as well. Acknowledged. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
