|
|
Created:
4 years, 8 months ago by servolk Modified:
4 years, 8 months ago Reviewers:
haraken, esprehn, philipj_slow, wolenetz CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, mlamouri+watch-blink_chromium.org, philipj_slow Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up WebSourceBufferClient interface
Use a struct instead of tuple for media track info.
Use a WebVector instead of std::vector in blink interface.
Committed: https://crrev.com/da2def6a572055b921d6559ad33366524d26d9ef
Cr-Commit-Position: refs/heads/master@{#388348}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove unused blinkTrackId variable for now #
Total comments: 2
Created: 4 years, 8 months ago
Messages
Total messages: 22 (9 generated)
Description was changed from ========== Clean up WebSourceBufferClient interface Use a struct instead of tuple for media track info. Use a WebVector instead of std::vector in blink interface. ========== to ========== Clean up WebSourceBufferClient interface Use a struct instead of tuple for media track info. Use a WebVector instead of std::vector in blink interface. ==========
servolk@chromium.org changed reviewers: + esprehn@chromium.org, philipj@opera.com, wolenetz@chromium.org
https://codereview.chromium.org/1897533002/diff/1/media/blink/websourcebuffer... File media/blink/websourcebuffer_impl.cc (right): https://codereview.chromium.org/1897533002/diff/1/media/blink/websourcebuffer... media/blink/websourcebuffer_impl.cc:191: blink::WebVector<blink::WebMediaPlayer::TrackId> blinkTrackIds = Whats the purpose of this if we just ignore the value?
https://codereview.chromium.org/1897533002/diff/1/media/blink/websourcebuffer... File media/blink/websourcebuffer_impl.cc (right): https://codereview.chromium.org/1897533002/diff/1/media/blink/websourcebuffer... media/blink/websourcebuffer_impl.cc:191: blink::WebVector<blink::WebMediaPlayer::TrackId> blinkTrackIds = On 2016/04/15 20:56:00, esprehn wrote: > Whats the purpose of this if we just ignore the value? This information is necessary to be able to map blink track id values to DemuxerStream* pointers in media::Pipeline. The previous CL that added this code implemented plumbing to get media track info to blink and then this set of track ids out of blink. The next CL that uses this set of track ids has not landed yet (although it will hopefully land soon, see https://codereview.chromium.org/1812543003/)
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/1897533002/diff/1/media/blink/websourcebuffer... File media/blink/websourcebuffer_impl.cc (right): https://codereview.chromium.org/1897533002/diff/1/media/blink/websourcebuffer... media/blink/websourcebuffer_impl.cc:191: blink::WebVector<blink::WebMediaPlayer::TrackId> blinkTrackIds = On 2016/04/15 21:02:32, servolk wrote: > On 2016/04/15 20:56:00, esprehn wrote: > > Whats the purpose of this if we just ignore the value? > > This information is necessary to be able to map blink track id values to > DemuxerStream* pointers in media::Pipeline. The previous CL that added this code > implemented plumbing to get media track info to blink and then this set of track > ids out of blink. The next CL that uses this set of track ids has not landed yet > (although it will hopefully land soon, see > https://codereview.chromium.org/1812543003/) I'd suggest you add the |blinkTrackIds| in the next CL that actually uses it.
Also LGTM % introducing things when they're used. Sorry I wasted your time with the tuple idea.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1897533002/#ps20001 (title: "Remove unused blinkTrackId variable for now")
On 2016/04/19 09:22:05, philipj_slow wrote: > Also LGTM % introducing things when they're used. Sorry I wasted your time with > the tuple idea. Ok, I've removed the unused variable in patchset #2
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1897533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897533002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Sorry for late nit :) LGTM % it (and if you need to fix the nit in follow-on CL, that's fine). https://codereview.chromium.org/1897533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1897533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:522: result[resultIdx++] = ++nextTrackId; nit: I'm not sure it's really nextTrackId, since pre-increment causes the stable state of this variable to store the nextTrackId_minus_one value in reality. Maybe initialize to 1, and post-increment it here so that its semantic matches its name?
https://codereview.chromium.org/1897533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/1897533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:522: result[resultIdx++] = ++nextTrackId; On 2016/04/19 22:35:02, wolenetz wrote: > nit: I'm not sure it's really nextTrackId, since pre-increment causes the stable > state of this variable to store the nextTrackId_minus_one value in reality. > Maybe initialize to 1, and post-increment it here so that its semantic matches > its name? As I've explained offline this is actually to emulate what blink nextTrackId is doing. see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... so I believe it's ok for now.
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/1897533002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897533002/20001
Message was sent while issue was closed.
Description was changed from ========== Clean up WebSourceBufferClient interface Use a struct instead of tuple for media track info. Use a WebVector instead of std::vector in blink interface. ========== to ========== Clean up WebSourceBufferClient interface Use a struct instead of tuple for media track info. Use a WebVector instead of std::vector in blink interface. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Clean up WebSourceBufferClient interface Use a struct instead of tuple for media track info. Use a WebVector instead of std::vector in blink interface. ========== to ========== Clean up WebSourceBufferClient interface Use a struct instead of tuple for media track info. Use a WebVector instead of std::vector in blink interface. Committed: https://crrev.com/da2def6a572055b921d6559ad33366524d26d9ef Cr-Commit-Position: refs/heads/master@{#388348} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/da2def6a572055b921d6559ad33366524d26d9ef Cr-Commit-Position: refs/heads/master@{#388348} |