Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1678523003:
Implement InitSegmentReceived algorithm in blink (Closed)
|
Created:
4 years, 10 months ago by servolk Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews, feature-media-reviews_chromium.org, philipj_slow, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com, chcunningham Base URL:
https://chromium.googlesource.com/chromium/src.git@blink-sb-audiotrack Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement InitSegmentReceived algorithm in blink
This CL begins moving MSE init segment received algorithm implementation
to blink level, so that it could be shared across different
implementations (e.g. between Chrome and Opera). The old init segment
algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to
be kept, since it does other important things that must be done on the
Chromium media pipeline level atm (e.g. creating track buffers and
demuxer streams).
BUG=620881
Committed: https://crrev.com/efae0344beeff450bbd5c9d1bff69c3d5d6f7104
Cr-Commit-Position: refs/heads/master@{#403286}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : update #Patch Set 8 : Set track.sourceBuffer #Patch Set 9 : Fix for when av tracks feature is disabled #Patch Set 10 : Added track id checks #Patch Set 11 : buildfix #Patch Set 12 : Minor updates #Patch Set 13 : Implement defaultTrackLanguage alg, use it for track language selection #Patch Set 14 : Added defaultTrackLabel algorithm, use it for track labels #Patch Set 15 : Added layout test for init segment algorithm + updated some comments #Patch Set 16 : rebase #Patch Set 17 : Rebase + update #
Total comments: 30
Patch Set 18 : CR feedback #Patch Set 19 : refactoring #Patch Set 20 : rebase #
Total comments: 11
Patch Set 21 : Updated test #Patch Set 22 : rebase #Patch Set 23 : rebase #Patch Set 24 : rebase #Patch Set 25 : Extend tests #
Total comments: 6
Patch Set 26 : Added track language tests #
Total comments: 1
Patch Set 27 : typo #Patch Set 28 : rebase #
Total comments: 13
Patch Set 29 : Don't check track ids in tests #Messages
Total messages: 60 (17 generated)
Description was changed from ========== Implement InitSegmentReceived algorithm in blink ========== to ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). ==========
servolk@google.com changed reviewers: + philipj@opera.com, wolenetz@chromium.org
The bots look very unhappy. There's a strange change in webexposed/global-interface-listing-expected.txt that I don't quite understand how it got there. Is there no code at all that should be removed from the Chromium side as part of this? If no observable change at all is expected, can you point out which tests cover this change?
On 2016/02/23 03:48:22, philipj_sick_March30 wrote: > The bots look very unhappy. There's a strange change in > webexposed/global-interface-listing-expected.txt that I don't quite understand > how it got there. > > Is there no code at all that should be removed from the Chromium side as part of > this? If no observable change at all is expected, can you point out which tests > cover this change? Is this change ready for review? Please rebase and rerun bots to hopefully show a better bot outcome :)
On 2016/03/30 20:28:18, wolenetz wrote: > On 2016/02/23 03:48:22, philipj_sick_March30 wrote: > > The bots look very unhappy. There's a strange change in > > webexposed/global-interface-listing-expected.txt that I don't quite understand > > how it got there. > > > > Is there no code at all that should be removed from the Chromium side as part > of > > this? If no observable change at all is expected, can you point out which > tests > > cover this change? > > Is this change ready for review? Please rebase and rerun bots to hopefully show > a better bot outcome :) I have updated this CL to work with Chromium top-of-master. It's not fully complete yet (there are still a few TODOs and not all of the 'init algorithm' are implemented, plus it needs more layout tests). But I think it gives a pretty good idea of the overall structure of the new algorithm and brings us much closer to being MSE-spec compliant. So feel free to take a look if you get a chance.
This is my last week at Opera, so I probably won't have time to see this through to landing, but left some comments anyway. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:505: const TrackDefault* SourceBuffer::getTrackDefaultHelper(const AtomicString& trackType, const AtomicString& byteStreamTrackID) const Maybe just getTrackDefault as the name? https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:514: const auto* trackDefault = m_trackDefaults->item(i); From http://chromium-cpp.appspot.com and https://google.github.io/styleguide/cppguide.html#auto I think this ought to be just const TrackDefault*. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:515: ASSERT(trackDefault); Not sure if there's a document rule here, but in https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ti040zXE_A8... most who weigh in don't think we should DCHECK immediately before dereferencing, and it generally isn't done at least in Blink. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:525: if (trackDefault->type() == trackType && trackDefault->byteStreamTrackID() == "") In the first loop you could maintain two variables and then just return one of those to avoid the second loop. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:574: std::vector<MediaTrackInfo> newAudioTracks; Vector<> https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:576: unsigned resultIdx = 0; size_t to match WebVector.h and the new usage above? https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:582: ASSERT(audioTrack); It seems a bit odd to assume the tracks are all there when the tracksMatchFirstInitSegment are all about checking that things add up. Is it really guaranteed that there will be an existing track with the same ID, no matter how one uses the web-exposed API? https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:594: ASSERT_NOT_REACHED(); NOTREACHED() is the new ASSERT_NOT_REACHED() https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:616: // - The codecs for each track, match what was specified in the first initialization segment. Is this also in MediaSourceState::OnNewConfigs? If so can you add a comment here? https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:622: tracksMatchFirstInitSegment = false; Also break? https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:630: tracksMatchFirstInitSegment = false; Ditto. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:656: for (const auto& trackInfo : newAudioTracks) { s/auto/MediaTrackInfo/ https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:658: const auto& byteStreamTrackId = trackInfo.byteStreamTrackId; s/auto/WebString/ and I think you could just use const WebString& for the locals below as well. At any rate using the same type for these seems sensible. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:663: if (language == "" || language == "und") language.isEmpty() would also cover the case where language.isNull() is true, if that can ever happen. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:664: language = defaultTrackLanguage("audio", byteStreamTrackId); Can you expose audioKeyword() from TrackDefault.cpp instead? TextTrack.h has a few of this variety.
https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:505: const TrackDefault* SourceBuffer::getTrackDefaultHelper(const AtomicString& trackType, const AtomicString& byteStreamTrackID) const On 2016/04/25 13:03:42, philipj_slow wrote: > Maybe just getTrackDefault as the name? Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:514: const auto* trackDefault = m_trackDefaults->item(i); On 2016/04/25 13:03:42, philipj_slow wrote: > From http://chromium-cpp.appspot.com and > https://google.github.io/styleguide/cppguide.html#auto I think this ought to be > just const TrackDefault*. Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:515: ASSERT(trackDefault); On 2016/04/25 13:03:42, philipj_slow wrote: > Not sure if there's a document rule here, but in > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/ti040zXE_A8... > most who weigh in don't think we should DCHECK immediately before dereferencing, > and it generally isn't done at least in Blink. Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:525: if (trackDefault->type() == trackType && trackDefault->byteStreamTrackID() == "") On 2016/04/25 13:03:42, philipj_slow wrote: > In the first loop you could maintain two variables and then just return one of > those to avoid the second loop. Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:574: std::vector<MediaTrackInfo> newAudioTracks; On 2016/04/25 13:03:42, philipj_slow wrote: > Vector<> Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:576: unsigned resultIdx = 0; On 2016/04/25 13:03:42, philipj_slow wrote: > size_t to match WebVector.h and the new usage above? Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:582: ASSERT(audioTrack); On 2016/04/25 13:03:42, philipj_slow wrote: > It seems a bit odd to assume the tracks are all there when the > tracksMatchFirstInitSegment are all about checking that things add up. Is it > really guaranteed that there will be an existing track with the same ID, no > matter how one uses the web-exposed API? Yeah, I guess strictly speaking it's better to handle that the same as mismatching track ids in subsequent init segments. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:594: ASSERT_NOT_REACHED(); On 2016/04/25 13:03:42, philipj_slow wrote: > NOTREACHED() is the new ASSERT_NOT_REACHED() Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:616: // - The codecs for each track, match what was specified in the first initialization segment. On 2016/04/25 13:03:42, philipj_slow wrote: > Is this also in MediaSourceState::OnNewConfigs? If so can you add a comment > here? Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:622: tracksMatchFirstInitSegment = false; On 2016/04/25 13:03:42, philipj_slow wrote: > Also break? Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:630: tracksMatchFirstInitSegment = false; On 2016/04/25 13:03:42, philipj_slow wrote: > Ditto. Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:656: for (const auto& trackInfo : newAudioTracks) { On 2016/04/25 13:03:42, philipj_slow wrote: > s/auto/MediaTrackInfo/ Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:658: const auto& byteStreamTrackId = trackInfo.byteStreamTrackId; On 2016/04/25 13:03:42, philipj_slow wrote: > s/auto/WebString/ and I think you could just use const WebString& for the locals > below as well. At any rate using the same type for these seems sensible. Done (using WebString). We can't use const reference, because we need to reassign it, in case we need to use the value from TrackDefaults. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:663: if (language == "" || language == "und") On 2016/04/25 13:03:42, philipj_slow wrote: > language.isEmpty() would also cover the case where language.isNull() is true, if > that can ever happen. Done. https://codereview.chromium.org/1678523003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:664: language = defaultTrackLanguage("audio", byteStreamTrackId); On 2016/04/25 13:03:42, philipj_slow wrote: > Can you expose audioKeyword() from TrackDefault.cpp instead? TextTrack.h has a > few of this variety. Done.
philipj@opera.com changed reviewers: - philipj@opera.com
I'll leave the rest of this review to wolenetz@
On 2016/04/26 14:34:32, philipj_slow wrote: > I'll leave the rest of this review to wolenetz@ Thanks Philip - I'll pick this up when I'm back on Tuesday 5/10.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678523003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678523003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/05/04 22:56:27, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) ping
On 2016/05/11 18:48:47, servolk wrote: > On 2016/05/04 22:56:27, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > ping Yep - on my list. I'll get to this by mid-day tomorrow at latest. Thanks for patience.
Here is a first round of comments. I'll follow up tomorrow with more in-depth review of the non-layout-test portion of this CL. Thanks again for your patience. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html (right): https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:7: <link rel="stylesheet" href="/w3c/resources/testharness.css"> nit: drop this link -- it's auto-included already by the included scripts, above. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:10: <div id="log"></div> nit: drop this div. unnecessary legacy cruft copy-pasted too much still :) https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:15: var initSegment = MediaSourceUtil.extractSegmentData(mediaData, segmentInfo.init); nit: 4-space script indent here and below. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:16: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); nit: use ' or " consistently within the script body, here and below. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:27: assert_equals(mediaElement.duration, 6.042); nit: use the field from segmentInfo, not hardcoding here. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:58: Please add testing of the application of TrackDefaults to inband tracks that are missing inband track language, label, or kind; include matching (and lack of matching) with and without specific byteStreamTrackID cases for more complete coverage of "Default track {language,label,kinds}" implementations.
Per recommendation from servolk@, further CR is pending his (in-progress) rework and landing of https://codereview.chromium.org/1922333002/ (which changes the way we are going to map track ids to demuxer streams).
On 2016/06/07 20:37:52, wolenetz_slow_reviews wrote: > Per recommendation from servolk@, further CR is pending his (in-progress) rework > and landing of https://codereview.chromium.org/1922333002/ (which changes the > way we are going to map track ids to demuxer streams). servolk@, please reference a bug (file a new one if needed that's blocking 249428) to track getting compliant initsegmentreceived algorithm landed.
On 2016/06/08 20:42:15, wolenetz wrote: > On 2016/06/07 20:37:52, wolenetz_slow_reviews wrote: > > Per recommendation from servolk@, further CR is pending his (in-progress) > rework > > and landing of https://codereview.chromium.org/1922333002/ (which changes the > > way we are going to map track ids to demuxer streams). > > servolk@, please reference a bug (file a new one if needed that's blocking > 249428) to track getting compliant initsegmentreceived algorithm landed. Dependency changed from https://codereview.chromium.org/1922333002/ to https://codereview.chromium.org/2050043002/, which landed earlier today. I'll review this shortly. Meanwhile, servolk@, please reference a bug (file a new one if needed that's blocking 249428) to track getting compliant initsegmentreceived algorithm landed.
Description was changed from ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). ========== to ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). BUG=620881 ==========
On 2016/06/16 19:57:45, wolenetz wrote: > On 2016/06/08 20:42:15, wolenetz wrote: > > On 2016/06/07 20:37:52, wolenetz_slow_reviews wrote: > > > Per recommendation from servolk@, further CR is pending his (in-progress) > > rework > > > and landing of https://codereview.chromium.org/1922333002/ (which changes > the > > > way we are going to map track ids to demuxer streams). > > > > servolk@, please reference a bug (file a new one if needed that's blocking > > 249428) to track getting compliant initsegmentreceived algorithm landed. > > Dependency changed from https://codereview.chromium.org/1922333002/ to > https://codereview.chromium.org/2050043002/, which landed earlier today. I'll > review this shortly. > > Meanwhile, servolk@, please reference a bug (file a new one if needed that's > blocking 249428) to track getting compliant initsegmentreceived algorithm > landed. Opened crbug.com/620881 for this. I've rebased this CL and slightly updated it. Still need to extend test coverage even more. Also in the current version of this CL we still don't handle track kinds yet. wip.
https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:598: if (!trackDefaultWithEmptyBytestreamId && trackDefault->byteStreamTrackID() == "") { nit: {} inconsistency within this method. Why using in this `if`, but not other `ifs`, above? https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:634: // Implementation of Initialization Segment Received, see 3.5.8 at nit: s/3.5.8 at// https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:658: appendError(true); Hmm. While I think this will probably work, our error reporting path here is a bit complex: If parse error occurs during parsing by the engine (but outside of initsegmentreceived algo here), I don't think we call appendError(true). We probably have a gap in our impl outside of this CL around this. I'm taking a diversion on this CR to see if we have such a gap. I'll get back to this CR (and your dynamic track enable/disable CR) afterwards.
On 2016/06/16 22:15:34, wolenetz wrote: > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:598: if > (!trackDefaultWithEmptyBytestreamId && trackDefault->byteStreamTrackID() == "") > { > nit: {} inconsistency within this method. Why using in this `if`, but not other > `ifs`, above? > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:634: // > Implementation of Initialization Segment Received, see 3.5.8 at > nit: s/3.5.8 at// > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:658: > appendError(true); > Hmm. While I think this will probably work, our error reporting path here is a > bit complex: > > If parse error occurs during parsing by the engine (but outside of > initsegmentreceived algo here), I don't think we call appendError(true). We > probably have a gap in our impl outside of this CL around this. > > I'm taking a diversion on this CR to see if we have such a gap. I'll get back to > this CR (and your dynamic track enable/disable CR) afterwards. Indeed there is a gap in our append error algorithm implementation for appendBuffer. For your CL, please just return false and don't directly call appendError(true) from initsegmentreceivedalgo in Blink SourceBuffer. It'll need to rebase on top of a fix for the gap that I'll be producing shortly. Bug for tracking this gap is https://bugs.chromium.org/p/chromium/issues/detail?id=620903
On 2016/06/16 23:00:04, wolenetz wrote: > On 2016/06/16 22:15:34, wolenetz wrote: > > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > > > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:598: if > > (!trackDefaultWithEmptyBytestreamId && trackDefault->byteStreamTrackID() == > "") > > { > > nit: {} inconsistency within this method. Why using in this `if`, but not > other > > `ifs`, above? > > > > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:634: // > > Implementation of Initialization Segment Received, see 3.5.8 at > > nit: s/3.5.8 at// > > > > > https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:658: > > appendError(true); > > Hmm. While I think this will probably work, our error reporting path here is a > > bit complex: > > > > If parse error occurs during parsing by the engine (but outside of > > initsegmentreceived algo here), I don't think we call appendError(true). We > > probably have a gap in our impl outside of this CL around this. > > > > I'm taking a diversion on this CR to see if we have such a gap. I'll get back > to > > this CR (and your dynamic track enable/disable CR) afterwards. > > Indeed there is a gap in our append error algorithm implementation for > appendBuffer. For your CL, please just return false and don't directly call > appendError(true) from initsegmentreceivedalgo in Blink SourceBuffer. It'll need > to rebase on top of a fix for the gap that I'll be producing shortly. Bug for > tracking this gap is > https://bugs.chromium.org/p/chromium/issues/detail?id=620903 Please see https://codereview.chromium.org/2076673005/, which I expect (once it lands) to impact this CL as follows: Any failure in SourceBuffer initsegmentreceivedalgorithm needs to return false back to the caller and that will (once https://codereview.chromium.org/2076673005/ lands) cause appendError(true) to occur because the failure of the initsegmentreceivedalgorithm becomes a failure of the appendData call. tl;dr: this CL's calls directly from the initsegmentreceived algorithm to appendError need to go away and be replaced by just "return false" back to OnNewConfigs(), which should similarly return false back to the parser to trigger the error handling path leading eventually to appendError(true). servolk@, please update this CL accordingly. Thanks for your patience while I worked this out.
https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html (right): https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:7: <link rel="stylesheet" href="/w3c/resources/testharness.css"> On 2016/05/12 20:42:04, wolenetz wrote: > nit: drop this link -- it's auto-included already by the included scripts, > above. Done. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:10: <div id="log"></div> On 2016/05/12 20:42:04, wolenetz wrote: > nit: drop this div. unnecessary legacy cruft copy-pasted too much still :) Done. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:15: var initSegment = MediaSourceUtil.extractSegmentData(mediaData, segmentInfo.init); On 2016/05/12 20:42:04, wolenetz wrote: > nit: 4-space script indent here and below. Done. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:16: test.expectEvent(sourceBuffer.audioTracks, 'addtrack', 'sourceBuffer.videoTracks addtrack event'); On 2016/05/12 20:42:04, wolenetz wrote: > nit: use ' or " consistently within the script body, here and below. Done. https://codereview.chromium.org/1678523003/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:27: assert_equals(mediaElement.duration, 6.042); On 2016/05/12 20:42:04, wolenetz wrote: > nit: use the field from segmentInfo, not hardcoding here. Done. https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:598: if (!trackDefaultWithEmptyBytestreamId && trackDefault->byteStreamTrackID() == "") { On 2016/06/16 22:15:33, wolenetz wrote: > nit: {} inconsistency within this method. Why using in this `if`, but not other > `ifs`, above? Done. https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:634: // Implementation of Initialization Segment Received, see 3.5.8 at On 2016/06/16 22:15:33, wolenetz wrote: > nit: s/3.5.8 at// Done. https://codereview.chromium.org/1678523003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:658: appendError(true); On 2016/06/16 22:15:33, wolenetz wrote: > Hmm. While I think this will probably work, our error reporting path here is a > bit complex: > > If parse error occurs during parsing by the engine (but outside of > initsegmentreceived algo here), I don't think we call appendError(true). We > probably have a gap in our impl outside of this CL around this. > > I'm taking a diversion on this CR to see if we have such a gap. I'll get back to > this CR (and your dynamic track enable/disable CR) afterwards. > Acknowledged. Ok, yes, I think reporting this error at the top level of appendBuffer/appendStream makes more sense. I'll replace the explicit appendError call with a comment about this.
https://codereview.chromium.org/1678523003/diff/500001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html (right): https://codereview.chromium.org/1678523003/diff/500001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-avtracks.html:25: assert_equals(sourceBuffer.videoTracks[0].language, "", "videoTrack.language"); Note: I've updated the default test.webm file to not specify the language of audio and video tracks explicitly, that makes it easier to test TrackDefaults and this was the only other test that actually cared about track languages.
servolk@chromium.org changed reviewers: + chcunningham@chromium.org
I haven't forgotten this :) https://codereview.chromium.org/2076673005/ is nearing landing (hopefully it'll get there tomorrow). Perhaps rebase this on its most recent patch set to make sure I didn't break you.
On 2016/06/21 23:47:44, wolenetz wrote: > I haven't forgotten this :) > > https://codereview.chromium.org/2076673005/ is nearing landing (hopefully it'll > get there tomorrow). > Perhaps rebase this on its most recent patch set to make sure I didn't break > you. Ok, I've rebased this on top of that change. PTAL.
servolk@chromium.org changed reviewers: + foolip@chromium.org
On 2016/06/22 23:32:06, servolk wrote: > On 2016/06/21 23:47:44, wolenetz wrote: > > I haven't forgotten this :) > > > > https://codereview.chromium.org/2076673005/ is nearing landing (hopefully > it'll > > get there tomorrow). > > Perhaps rebase this on its most recent patch set to make sure I didn't break > > you. > > Ok, I've rebased this on top of that change. PTAL. +foolip@ for third_party/WebKit/public/platform/WebSourceBufferClient.h
third_party/WebKit/public/platform/WebSourceBufferClient.h rs lgtm, I haven't revisted any of the old discussion.
wolenetz and I have agreed to divide and conquer. He'll take this one; moving me to cc.
chcunningham@chromium.org changed reviewers: - chcunningham@chromium.org
lgtm % making the unique ID generation verification less specific to Chrome in the layout test Thank you! https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html (right): https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: <script src="/w3c/resources/testharness.js"></script> nits: Since this is going shortly to upstream w-p-t, let's adjust the style before landing: no <html>, <head>, <body>, 0-based indentation of outermost level of script body. If this is all there is, no worries - I'll bulk update style as part of pushing to wpt (IOW, no strong need to fix this style in this CL). https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: "" }; Hmm. This may be too specific to our implementation to be directly usable as a cross-platform compliance test. id must simply be unique. Can you make the verification of id be more generic here? https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not generated), so these should be interoperable (unlike the generated track ID expectations in my comment, above.) https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:90: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", language: "audio-language" }; This "2" is a generated track ID, not the same as the "2" bytestreamTrackID, above. Coincidentally, they match in this test with our Chrome ID generation code, right? If so, please again make the track ID (not bytestreamTrackID) verification more generic (and interoperable) here (and in next line). https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:105: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", language: "audio-language" }; ditto: more generic track ID uniqueness verification, unbound to chrome-specific ID generation scheme, please.
On 2016/06/27 19:31:07, wolenetz wrote: > lgtm % making the unique ID generation verification less specific to Chrome in > the layout test > > Thank you! > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html > (right): > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: > <script src="/w3c/resources/testharness.js"></script> > nits: Since this is going shortly to upstream w-p-t, let's adjust the style > before landing: > no <html>, <head>, <body>, 0-based indentation of outermost level of script > body. > > If this is all there is, no worries - I'll bulk update style as part of pushing > to wpt (IOW, no strong need to fix this style in this CL). > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: "" }; > Hmm. This may be too specific to our implementation to be directly usable as a > cross-platform compliance test. > id must simply be unique. Can you make the verification of id be more generic > here? > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: > new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), > aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not > generated), so these should be interoperable (unlike the generated track ID > expectations in my comment, above.) > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:90: > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > language: "audio-language" }; > This "2" is a generated track ID, not the same as the "2" bytestreamTrackID, > above. Coincidentally, they match in this test with our Chrome ID generation > code, right? If so, please again make the track ID (not bytestreamTrackID) > verification more generic (and interoperable) here (and in next line). > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:105: > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > language: "audio-language" }; > ditto: more generic track ID uniqueness verification, unbound to chrome-specific > ID generation scheme, please. Also, does Chrome pass your tests if using test.mp4 (you can force this locally by disabling audio/webm and video/webm support (e.g. by renaming them to "audio/webmdisabled"/"video/webmdisabled" locally only in your media/filters/stream_parser_factory.cc and building with proprietary_codecs=1 and ffmpeg_branding=Chrome or ChromeOS))?
On 2016/06/27 19:34:59, wolenetz wrote: > On 2016/06/27 19:31:07, wolenetz wrote: > > lgtm % making the unique ID generation verification less specific to Chrome in > > the layout test > > > > Thank you! > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html > > (right): > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: > > <script src="/w3c/resources/testharness.js"></script> > > nits: Since this is going shortly to upstream w-p-t, let's adjust the style > > before landing: > > no <html>, <head>, <body>, 0-based indentation of outermost level of script > > body. > > > > If this is all there is, no worries - I'll bulk update style as part of > pushing > > to wpt (IOW, no strong need to fix this style in this CL). > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: "" > }; > > Hmm. This may be too specific to our implementation to be directly usable as a > > cross-platform compliance test. > > id must simply be unique. Can you make the verification of id be more generic > > here? > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: > > new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), > > aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not > > generated), so these should be interoperable (unlike the generated track ID > > expectations in my comment, above.) > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:90: > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > > language: "audio-language" }; > > This "2" is a generated track ID, not the same as the "2" bytestreamTrackID, > > above. Coincidentally, they match in this test with our Chrome ID generation > > code, right? If so, please again make the track ID (not bytestreamTrackID) > > verification more generic (and interoperable) here (and in next line). > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:105: > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > > language: "audio-language" }; > > ditto: more generic track ID uniqueness verification, unbound to > chrome-specific > > ID generation scheme, please. > > Also, does Chrome pass your tests if using test.mp4 (you can force this locally > by disabling audio/webm and video/webm support (e.g. by renaming them to > "audio/webmdisabled"/"video/webmdisabled" locally only in your > media/filters/stream_parser_factory.cc and building with proprietary_codecs=1 > and ffmpeg_branding=Chrome or ChromeOS))? Ping :) Is there anything you need from us to help get this landed? Thanks, Matt
On 2016/06/29 21:23:21, wolenetz wrote: > On 2016/06/27 19:34:59, wolenetz wrote: > > On 2016/06/27 19:31:07, wolenetz wrote: > > > lgtm % making the unique ID generation verification less specific to Chrome > in > > > the layout test > > > > > > Thank you! > > > > > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > File > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html > > > (right): > > > > > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: > > > <script src="/w3c/resources/testharness.js"></script> > > > nits: Since this is going shortly to upstream w-p-t, let's adjust the style > > > before landing: > > > no <html>, <head>, <body>, 0-based indentation of outermost level of script > > > body. > > > > > > If this is all there is, no worries - I'll bulk update style as part of > > pushing > > > to wpt (IOW, no strong need to fix this style in this CL). > > > > > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: > > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: > "" > > }; > > > Hmm. This may be too specific to our implementation to be directly usable as > a > > > cross-platform compliance test. > > > id must simply be unique. Can you make the verification of id be more > generic > > > here? > > > > > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: > > > new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), > > > aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not > > > generated), so these should be interoperable (unlike the generated track ID > > > expectations in my comment, above.) > > > > > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:90: > > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > > > language: "audio-language" }; > > > This "2" is a generated track ID, not the same as the "2" bytestreamTrackID, > > > above. Coincidentally, they match in this test with our Chrome ID generation > > > code, right? If so, please again make the track ID (not bytestreamTrackID) > > > verification more generic (and interoperable) here (and in next line). > > > > > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:105: > > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > > > language: "audio-language" }; > > > ditto: more generic track ID uniqueness verification, unbound to > > chrome-specific > > > ID generation scheme, please. > > > > Also, does Chrome pass your tests if using test.mp4 (you can force this > locally > > by disabling audio/webm and video/webm support (e.g. by renaming them to > > "audio/webmdisabled"/"video/webmdisabled" locally only in your > > media/filters/stream_parser_factory.cc and building with proprietary_codecs=1 > > and ffmpeg_branding=Chrome or ChromeOS))? > > Ping :) Is there anything you need from us to help get this landed? > > Thanks, > Matt Sorry, I'll get back to this shortly. I was busy with finishing the work necessary to get https://codereview.chromium.org/1935873002/ landed.
https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html (right): https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: <script src="/w3c/resources/testharness.js"></script> On 2016/06/27 19:31:06, wolenetz wrote: > nits: Since this is going shortly to upstream w-p-t, let's adjust the style > before landing: > no <html>, <head>, <body>, 0-based indentation of outermost level of script > body. > > If this is all there is, no worries - I'll bulk update style as part of pushing > to wpt (IOW, no strong need to fix this style in this CL). Since all existing tests under third_party/WebKit/LayoutTests/http/tests/media/media-source/ still use this old style, I'd prefer to keep it this way as well, for consistency. Bulk convert later sgtm. https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: "" }; On 2016/06/27 19:31:07, wolenetz wrote: > Hmm. This may be too specific to our implementation to be directly usable as a > cross-platform compliance test. > id must simply be unique. Can you make the verification of id be more generic > here? I'm not sure what exactly you mean by 'make it more generic'. Is there anything, besides track id uniqueness, that we can check without relying on track ids having specific values? I guess we could remove explicit id comparisons and keep just the id uniqueness check. Is that what you meant? https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), On 2016/06/27 19:31:07, wolenetz wrote: > aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not > generated), so these should be interoperable (unlike the generated track ID > expectations in my comment, above.) Acknowledged. https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), On 2016/06/27 19:31:07, wolenetz wrote: > aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not > generated), so these should be interoperable (unlike the generated track ID > expectations in my comment, above.) Acknowledged. https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:90: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", language: "audio-language" }; On 2016/06/27 19:31:07, wolenetz wrote: > This "2" is a generated track ID, not the same as the "2" bytestreamTrackID, > above. Coincidentally, they match in this test with our Chrome ID generation > code, right? If so, please again make the track ID (not bytestreamTrackID) > verification more generic (and interoperable) here (and in next line). Acknowledged. https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:105: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", language: "audio-language" }; On 2016/06/27 19:31:07, wolenetz wrote: > ditto: more generic track ID uniqueness verification, unbound to chrome-specific > ID generation scheme, please. Acknowledged.
On 2016/06/27 19:34:59, wolenetz wrote: > On 2016/06/27 19:31:07, wolenetz wrote: > > lgtm % making the unique ID generation verification less specific to Chrome in > > the layout test > > > > Thank you! > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html > > (right): > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: > > <script src="/w3c/resources/testharness.js"></script> > > nits: Since this is going shortly to upstream w-p-t, let's adjust the style > > before landing: > > no <html>, <head>, <body>, 0-based indentation of outermost level of script > > body. > > > > If this is all there is, no worries - I'll bulk update style as part of > pushing > > to wpt (IOW, no strong need to fix this style in this CL). > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: "" > }; > > Hmm. This may be too specific to our implementation to be directly usable as a > > cross-platform compliance test. > > id must simply be unique. Can you make the verification of id be more generic > > here? > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:84: > > new TrackDefault("audio", "audio-language", "audio-label", ["main"], "2"), > > aside: these bytestreamTrackIDs ("2" and "1") are parsed from the media (not > > generated), so these should be interoperable (unlike the generated track ID > > expectations in my comment, above.) > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:90: > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > > language: "audio-language" }; > > This "2" is a generated track ID, not the same as the "2" bytestreamTrackID, > > above. Coincidentally, they match in this test with our Chrome ID generation > > code, right? If so, please again make the track ID (not bytestreamTrackID) > > verification more generic (and interoperable) here (and in next line). > > > > > https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:105: > > var expectedAudioTrackInfo = { id: "2", kind: "main", label: "audio-label", > > language: "audio-language" }; > > ditto: more generic track ID uniqueness verification, unbound to > chrome-specific > > ID generation scheme, please. > > Also, does Chrome pass your tests if using test.mp4 (you can force this locally > by disabling audio/webm and video/webm support (e.g. by renaming them to > "audio/webmdisabled"/"video/webmdisabled" locally only in your > media/filters/stream_parser_factory.cc and building with proprietary_codecs=1 > and ffmpeg_branding=Chrome or ChromeOS))? I'll give it a try with the test.mp4 file. I guess it might fail atm, since the test.mp4 might specify track languages/labels explicitly and we rely on track languages being not defined in the test case for TrackDefaults. I'll see if I can update test.mp4 to make it work.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. test.mp4: please file a follow-up crbug to check that your tests work with mp4 mode. Thanks for doing all this work!! https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html (right): https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:4: <script src="/w3c/resources/testharness.js"></script> On 2016/06/29 22:23:25, servolk wrote: > On 2016/06/27 19:31:06, wolenetz wrote: > > nits: Since this is going shortly to upstream w-p-t, let's adjust the style > > before landing: > > no <html>, <head>, <body>, 0-based indentation of outermost level of script > > body. > > > > If this is all there is, no worries - I'll bulk update style as part of > pushing > > to wpt (IOW, no strong need to fix this style in this CL). > > Since all existing tests under > third_party/WebKit/LayoutTests/http/tests/media/media-source/ still use this old > style, I'd prefer to keep it this way as well, for consistency. Bulk convert > later sgtm. Acknowledged. And it appears the guidance I was given around this fails unless we have something like the "log div" there to trigger generation of the enclosing document. So please don't fix the style here and now :) https://codereview.chromium.org/1678523003/diff/540001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-initsegmentreceived-alg.html:55: var expectedAudioTrackInfo = { id: "2", kind: "main", label: "", language: "" }; On 2016/06/29 22:23:24, servolk wrote: > On 2016/06/27 19:31:07, wolenetz wrote: > > Hmm. This may be too specific to our implementation to be directly usable as a > > cross-platform compliance test. > > id must simply be unique. Can you make the verification of id be more generic > > here? > > I'm not sure what exactly you mean by 'make it more generic'. Is there anything, > besides track id uniqueness, that we can check without relying on track ids > having specific values? I guess we could remove explicit id comparisons and keep > just the id uniqueness check. Is that what you meant? Yes, that is what I meant. Sorry if confusing.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/1678523003/#ps560001 (title: "Don't check track ids in tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). BUG=620881 ========== to ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). BUG=620881 ==========
Message was sent while issue was closed.
Committed patchset #29 (id:560001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). BUG=620881 ========== to ========== Implement InitSegmentReceived algorithm in blink This CL begins moving MSE init segment received algorithm implementation to blink level, so that it could be shared across different implementations (e.g. between Chrome and Opera). The old init segment algorithm is in MediaSourceState::OnNewConfigs, and for now it needs to be kept, since it does other important things that must be done on the Chromium media pipeline level atm (e.g. creating track buffers and demuxer streams). BUG=620881 Committed: https://crrev.com/efae0344beeff450bbd5c9d1bff69c3d5d6f7104 Cr-Commit-Position: refs/heads/master@{#403286} ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/efae0344beeff450bbd5c9d1bff69c3d5d6f7104 Cr-Commit-Position: refs/heads/master@{#403286} |
