Closed
Bug 1096089
Opened 10 years ago
Closed 10 years ago
Implement MSE coded frame removal algorithm
Categories
(Core :: Audio/Video, defect, P2)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox35 | --- | unaffected |
firefox36 | --- | fixed |
firefox37 | --- | fixed |
firefox38 | --- | fixed |
firefox39 | --- | affected |
People
(Reporter: cajbir, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 8 obsolete files)
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
cajbir
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cajbir
:
review+
bholley
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The coded frame removal algorithm is defined in the MSE spec:
https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-coded-frame-removal
This is currently a TODO in the SourceBuffer implementation and needs to be implemented.
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
Initial incomplete implementation. Works with youtube, resolution change are now instantaneous
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
I'm moving this to P1 as it makes YouTube change of resolution non-working.
When changing resolution, YouTube calls remove(0, duration) on the entire current source buffer. As our remove does nothing, YouTube only adds new resolution data after the end of the current buffered range.
It also causes seek backward to return to the old low resolution.
In my case, YouTube starts playing most video in 144p, which buffers very quickly and if you change resolution after say 10s to 1080p, it has sufficiently buffered close to the entire video (here a 3' video clip).
So we only have 1080p video for the remaining couple of seconds.
Priority: P2 → P1
Assignee | ||
Comment 4•10 years ago
|
||
Quick partial range removal implementation. Data is only ever completely evicted if we are clearing the entire buffered range ; which is what YouTube does. In other cases, we only trim the data when end time is prior the current playback position.
Attachment #8552912 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Attachment #8552822 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8552912 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm
cajbit if you could review the eviction itself. I've pretty much copy/paste the existing eviction for when we're evicting before current playback position.
I have proper eviction as per spec on the way.
Attachment #8552912 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 6•10 years ago
|
||
My earlier patch was guaranteed to work only with the patches I have queued from bug 1118589. Must run synchronously if appendBuffer is synchronous.
Assignee | ||
Updated•10 years ago
|
Attachment #8552912 -
Attachment is obsolete: true
Attachment #8552912 -
Flags: review?(matt.woodrow)
Attachment #8552912 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8552999 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Attachment #8552999 -
Flags: review?(cajbir.bugzilla)
Comment 7•10 years ago
|
||
Comment on attachment 8552999 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm
Review of attachment 8552999 [details] [diff] [review]:
-----------------------------------------------------------------
Looks pretty good! Not finishing the review yet, since you said there were still bugs to be fixed.
::: dom/media/mediasource/SourceBuffer.cpp
@@ +259,5 @@
> + // abort was called in between.
> + return;
> + }
> + if (mTrackBuffer) {
> + int64_t start = IsInfinite(aStart) ? INT64_MAX : (int64_t)(aStart * USECS_PER_S);
Shouldn't we just return without doing anything if aStart is infinite?
::: dom/media/mediasource/SourceBufferDecoder.h
@@ +121,5 @@
> // cached data. Returns -1 if no such value is computable.
> int64_t ConvertToByteOffset(double aTime);
>
> + // all durations are in usecs
> + void Trim(int64_t aDuration);
Can you add some comments explaining that this is basically a hack to workaround the fact that we can't accurately remove coded frames at the moment. Also that we don't really remove any data, just 'hide' it and prevent playback beyond the trim point.
::: dom/media/mediasource/TrackBuffer.cpp
@@ +728,5 @@
> + ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
> +
> + nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> + double end = Buffered(buffered) * USECS_PER_S;
> + double start = buffered->GetStartTime() * USECS_PER_S;
naming of start/end and aStart/aEnd could be improved to make this clearer.
bufferedStart, removeStart maybe?
@@ +733,5 @@
> +
> + if (aStart >= end ||
> + (aEnd < end && aEnd >= aPlaybackTime)) {
> + // TODO. We only handle trimming and removal before current playback position.
> + return false;
Throw an NS_WARNING in here maybe? That should let us know when we hit this, so we don't spend time debugging failures caused by it.
@@ +762,5 @@
> + continue;
> + }
> + MSE_DEBUG("TrackBuffer(%p):RangeRemoval remove empty decoders=%d", this, i);
> + RemoveDecoder(decoders[i]);
> + }
Would be nice to share this code with EvictData
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8552999 [details] [diff] [review]
> MSE: Partially implement Range Removal algorithm
>
> Review of attachment 8552999 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks pretty good! Not finishing the review yet, since you said there were
> still bugs to be fixed.
the bugs are triggered by this patch, but I don't believe they are in this patch ...
On the VM where I can reproduce it, I have to change resolution a dozen times before it happens. so pretty minor I think.
I will submit another patch once I identify the problem in MDSM
>
> ::: dom/media/mediasource/SourceBuffer.cpp
> @@ +259,5 @@
> > + // abort was called in between.
> > + return;
> > + }
> > + if (mTrackBuffer) {
> > + int64_t start = IsInfinite(aStart) ? INT64_MAX : (int64_t)(aStart * USECS_PER_S);
>
> Shouldn't we just return without doing anything if aStart is infinite?
we could yes...
> Can you add some comments explaining that this is basically a hack to
> workaround the fact that we can't accurately remove coded frames at the
> moment. Also that we don't really remove any data, just 'hide' it and
> prevent playback beyond the trim point.
ok
> Would be nice to share this code with EvictData
ultimately, evict data is supposed to call RangeRemoval ...
Earlier on, all RangeRemoval did was trim, I added cutting from the beginning as an afterthought.
thanks for the review.
Assignee | ||
Comment 9•10 years ago
|
||
Only return END_OF_STREAM when decoding from an ended media source if we're near its end. This fixes audio continuing to play while video is frozen.
Attachment #8553470 -
Flags: review?(matt.woodrow)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8552999 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm
Review of attachment 8552999 [details] [diff] [review]:
-----------------------------------------------------------------
Trimming results in the Buffered value returning less than the actual data we are holding. This is used in the EvictData call to identify when to evict the data. We need some way of telling EvictData the actual data that is held so it knows when to evict. A GetActualBuffered() type of call it can use. I think this can be done in another bug - either create one for it or point to an existing bug the work can be done under.
::: dom/media/mediasource/MediaSourceReader.cpp
@@ +123,5 @@
> mAudioPromise.Reject(CANCELED, __func__);
> return p;
> }
> + SwitchReaderResult ret = SwitchAudioReader(mLastAudioTime);
> + if (ret == READER_ERROR) {
Use 'switch' and 'case' on the valid values of SwitchReaderResult.
@@ +228,1 @@
> if (SwitchAudioReader(mLastAudioTime, EOS_FUZZ_US)) {
This will succeed on READER_ERROR too - was that intended? The Video version later checks for READER_NEW.
@@ +258,5 @@
> mVideoPromise.Reject(CANCELED, __func__);
> return p;
> }
> + SwitchReaderResult ret = SwitchVideoReader(mLastVideoTime);
> + if (ret == READER_ERROR) {
Switch/case please.
::: dom/media/mediasource/SourceBuffer.cpp
@@ +35,5 @@
> #define MSE_DEBUGV(...)
> #define MSE_API(...)
> #endif
>
> +#define APPENDBUFFER_IS_SYNCHRONOUS 1
Comment why this is needed. Is it a workaround for something? Why would I set it to zero, or undefine it? Is it zero to change the behaviour or undefined changes the behaviour?
@@ +240,2 @@
>
> +#if APPENDBUFFER_IS_SYNCHRONOUS
I would prefer "#if defined(APPENDBUFFER_IS_SYNCHRONOUS") and undefined meaning the opposite, rather than setting to 1/0 but no matter which you choose, add a comment somewhere explaining which way it is.
::: dom/media/mediasource/SourceBuffer.h
@@ +110,5 @@
> double GetBufferedEnd();
>
> // Runs the range removal algorithm as defined by the MSE spec.
> void RangeRemoval(double aStart, double aEnd);
> + void DoRangeRemoval(double aStart, double aEnd);
Comment explaining difference between this and RangeRemoval.
::: dom/media/mediasource/SourceBufferDecoder.cpp
@@ +243,5 @@
> nsresult
> SourceBufferDecoder::GetBuffered(dom::TimeRanges* aBuffered)
> {
> + nsresult rv = mReader->GetBuffered(aBuffered);
> + NS_ENSURE_SUCCESS(rv, rv);
NS_ENSURE macros are asked not to be used in new code in the coding style. Change to:
if (NS_FAILED(rv)) {
return rv;
}
::: dom/media/mediasource/SourceBufferDecoder.h
@@ +120,5 @@
> // Given a time convert it into an approximate byte offset from the
> // cached data. Returns -1 if no such value is computable.
> int64_t ConvertToByteOffset(double aTime);
>
> + // all durations are in usecs
Capital A.
@@ +121,5 @@
> // cached data. Returns -1 if no such value is computable.
> int64_t ConvertToByteOffset(double aTime);
>
> + // all durations are in usecs
> + void Trim(int64_t aDuration);
Explain whether this is a TrimFromStart, or a TrimFromEnd.
@@ +127,5 @@
> + {
> + return mTrimmedOffset >= 0;
> + }
> +
> + void SetRealMediaDuration(int64_t aDuration);
Comment saying what a 'Real Media Duration' is vs a non-real media duration. Say what the units are. Same for return value of GetRealMediaDuration. You have an "all durations are in usecs" further above but I think it's better to have it listed with each function so people don't need to go searching for it.
@@ +148,2 @@
> int64_t mTimestampOffset;
> int64_t mMediaDuration;
Explain difference between mMediaDuration and mRealMediaDuration - either here or in the function as mentioned previously.
@@ +148,5 @@
> int64_t mTimestampOffset;
> int64_t mMediaDuration;
> + int64_t mRealMediaDuration;
> + // in seconds
> + double mTrimmedOffset;
Explain what exactly this is. Is it trimming before, or after the offset for example.
::: dom/media/mediasource/TrackBuffer.h
@@ +94,5 @@
>
> + // Runs MSE range removal algorithm.
> + // http://w3c.github.io/media-source/#sourcebuffer-coded-frame-removal
> + // Implementation is only partial, we can only trim a buffer, or remove
> + // data found prior the existing playback position.
"prior to the"
@@ +96,5 @@
> + // http://w3c.github.io/media-source/#sourcebuffer-coded-frame-removal
> + // Implementation is only partial, we can only trim a buffer, or remove
> + // data found prior the existing playback position.
> + // Returns true if data was evicted.
> + bool RangeRemoval(int64_t aPlaybackTime, int64_t aStart, int64_t aEnd);
Mention what units these arguments are in.
@@ +119,5 @@
> already_AddRefed<SourceBufferDecoder> NewDecoder(int64_t aTimestampOffset /* microseconds */);
>
> // Helper for AppendData, ensures NotifyDataArrived is called whenever
> // data is appended to the current decoder's SourceBufferResource.
> + bool AppendDataToCurrentResource(const uint8_t* aData, uint32_t aLength, uint32_t aDuration);
Mention what unit aDuration is.
Attachment #8552999 -
Flags: review?(cajbir.bugzilla) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Some quick explanations:
(In reply to cajbir (:cajbir) from comment #10)
> Use 'switch' and 'case' on the valid values of SwitchReaderResult.
ok.
>
> @@ +228,1 @@
> > if (SwitchAudioReader(mLastAudioTime, EOS_FUZZ_US)) {
>
> This will succeed on READER_ERROR too - was that intended? The Video version
> later checks for READER_NEW.
>
I simply keep the existing behaviour for code path my code do not affect. but yes, this should be done too (as I believe what we had was buggy)
> Comment why this is needed. Is it a workaround for something? Why would I
> set it to zero, or undefine it? Is it zero to change the behaviour or
> undefined changes the behaviour?
this is while waiting for bug 1118589 to land...
if appendbuffer is made asynchronous, then range removal must be too.
Thanks for the review..
Comment 12•10 years ago
|
||
Comment on attachment 8553470 [details] [diff] [review]
Only return end of stream if we're near the known duration
Review of attachment 8553470 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/mediasource/MediaSourceReader.cpp
@@ +940,5 @@
> +bool
> +MediaSourceReader::IsNearEnd(int64_t aTime)
> +{
> + ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> + return mEnded && aTime >= (mMediaSourceDuration * USECS_PER_S - EOS_FUZZ_US);
This seems like a large amount of fuzz.
Attachment #8553470 -
Flags: review?(matt.woodrow) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8552999 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm
Review of attachment 8552999 [details] [diff] [review]:
-----------------------------------------------------------------
Looks pretty good! Not finishing the review yet, since you said there were still bugs to be fixed.
::: dom/media/mediasource/SourceBuffer.cpp
@@ +259,5 @@
> + // abort was called in between.
> + return;
> + }
> + if (mTrackBuffer) {
> + int64_t start = IsInfinite(aStart) ? INT64_MAX : (int64_t)(aStart * USECS_PER_S);
Shouldn't we just return without doing anything if aStart is infinite?
::: dom/media/mediasource/SourceBufferDecoder.h
@@ +121,5 @@
> // cached data. Returns -1 if no such value is computable.
> int64_t ConvertToByteOffset(double aTime);
>
> + // all durations are in usecs
> + void Trim(int64_t aDuration);
Can you add some comments explaining that this is basically a hack to workaround the fact that we can't accurately remove coded frames at the moment. Also that we don't really remove any data, just 'hide' it and prevent playback beyond the trim point.
::: dom/media/mediasource/TrackBuffer.cpp
@@ +728,5 @@
> + ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
> +
> + nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> + double end = Buffered(buffered) * USECS_PER_S;
> + double start = buffered->GetStartTime() * USECS_PER_S;
naming of start/end and aStart/aEnd could be improved to make this clearer.
bufferedStart, removeStart maybe?
@@ +733,5 @@
> +
> + if (aStart >= end ||
> + (aEnd < end && aEnd >= aPlaybackTime)) {
> + // TODO. We only handle trimming and removal before current playback position.
> + return false;
Throw an NS_WARNING in here maybe? That should let us know when we hit this, so we don't spend time debugging failures caused by it.
@@ +762,5 @@
> + continue;
> + }
> + MSE_DEBUG("TrackBuffer(%p):RangeRemoval remove empty decoders=%d", this, i);
> + RemoveDecoder(decoders[i]);
> + }
Would be nice to share this code with EvictData
Attachment #8552999 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Carrying both r+. Thanks guys. Removed the trimming before the current playback position. It leaves the source buffer resource in unreadable state as both demuxers will choke on invalid data. EvictData must be re-written.
Assignee | ||
Updated•10 years ago
|
Attachment #8552999 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Rebase following earlier changes (switch vs if)
Assignee | ||
Updated•10 years ago
|
Attachment #8553470 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
Was failing the media-source/mediasource-duration.html webref.
It turns out they set a sourceBuffer with a timestampOffset = 2. Then call appendBuffer with 1s of data. So we have a buffered range of [2,3].
Request*Data was returning WAITING_FOR_DATA while it would have returned the first frame at 2s previously. The MediaDecoderStateMachine is waiting for a first frame to be decoded at time 0 immediately, so I added a test to check if we've already decoded one frame. If not, we default to the old behaviour of using the current decoder rather than return EOF or WAITING.
I don't think either behaviour is right. We still fail the test, just differently.
I wrote a quick test page: http://people.mozilla.org/~jyavenard/tests/mse_mp4/paper-offset.html?eos=1&duration=-1 and checked that our behaviour is the same between Chrome and IE.
This revealed an existing problem in the MDSM: it appears that once we hit WAITING_FOR_DATA we are then unable to seek. Chrome has no issue seeking within the 2-10s existing range.
Assignee | ||
Updated•10 years ago
|
Attachment #8553544 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
rebase
Assignee | ||
Updated•10 years ago
|
Attachment #8553548 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 19•10 years ago
|
||
Comment 20•10 years ago
|
||
Comment on attachment 8553654 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm
Review of attachment 8553654 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/mediasource/SourceBufferDecoder.cpp
@@ +249,5 @@
> + }
> + if (!WasTrimmed()) {
> + return NS_OK;
> + }
> + nsRefPtr<dom::TimeRanges> tr = new dom::TimeRanges();
Missing #include "mozilla/dom/TimeRanges.h"
Comment 21•10 years ago
|
||
Comment on attachment 8553654 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm
Requesting uplift for the first two patches on this bug, plus the unified build fix.
Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: YouTube playback can show lower quality video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This is moderately risky, but is MSE-specific. We'd like to try it for testing and see if it addresses issue with Firefox no switching to higher quality video properly.
[String/UUID change made/needed]: None.
Attachment #8553654 -
Flags: approval-mozilla-beta?
Attachment #8553654 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox35:
--- → unaffected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → fixed
Comment 22•10 years ago
|
||
Fix the non-unified build issue reported in comment #20. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/079ea32ad2b2
Attachment #8554634 -
Flags: review+
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Re-introduce code that I later removed. That's to evict data before current playback position. This was r+ already
Updated•10 years ago
|
Attachment #8553654 -
Flags: approval-mozilla-beta?
Attachment #8553654 -
Flags: approval-mozilla-beta+
Attachment #8553654 -
Flags: approval-mozilla-aurora?
Attachment #8553654 -
Flags: approval-mozilla-aurora+
Comment 25•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0d24eaaf835
https://hg.mozilla.org/releases/mozilla-aurora/rev/e73202798538
https://hg.mozilla.org/releases/mozilla-beta/rev/570a09a6eb68
https://hg.mozilla.org/releases/mozilla-beta/rev/c7db1d42b4b6
Setting the 36/37 statuses to fixed for now to get this bug off the needs-uplift radar. You can set them back to affected when there's more ready to go.
Assignee | ||
Comment 26•10 years ago
|
||
Update webref. Like bug 1125776, we fail due to bug 1065207. We're off by a few milliseconds in the duration reported on webm. And for mp4, I don't know where they get their 6s from (the init segment says it's 15s)
Attachment #8557429 -
Flags: review?(cajbir.bugzilla)
Reporter | ||
Comment 27•10 years ago
|
||
Comment on attachment 8557429 [details] [diff] [review]
Update webref tests
I defer to karl on web platform test changes.
Attachment #8557429 -
Flags: review?(cajbir.bugzilla) → review?(karlt)
Comment 28•10 years ago
|
||
Comment on attachment 8557429 [details] [diff] [review]
Update webref tests
I expect the changes in this bug haven't actually changed the results here, but I think the logic is better. (These tests don't run on Macs in automation yet.)
So I'd suggest something like "Generalize [Test remove with a start at the duration.] expected result to all platforms using mp4" for a comment.
Attachment #8557429 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 29•10 years ago
|
||
Carrying r+
Assignee | ||
Updated•10 years ago
|
Attachment #8557429 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
Comment 31•10 years ago
|
||
Updated•10 years ago
|
Priority: P1 → P2
Comment 32•10 years ago
|
||
Pushing third bit to 37.
https://hg.mozilla.org/releases/mozilla-aurora/rev/a610543801e0
Comment 33•10 years ago
|
||
Assuming we can close this now.
Updated•10 years ago
|
Target Milestone: --- → mozilla38
Assignee | ||
Comment 34•10 years ago
|
||
Add removal from beginning of source buffer. That adds support for both type of trimming (from left and from right). Adding support of removing data in the middle of the source buffer with our existing architecture is likely a moot point. This patch is very similar to the one you r+ earlier, except that it doesn't restrict to truncation before the current playback time as the MediaSourceReader will handle the condition properly
Attachment #8567711 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8555227 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8567711 -
Flags: review?(cajbir.bugzilla) → review+
Assignee | ||
Comment 35•10 years ago
|
||
This bug is about the implementation of Range Removal algorithm.
The final part about removing data in the middle of a source buffer isn't implemented yet
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 36•10 years ago
|
||
Be compliant with spec: end is an unrestricted double. Test for updating attribute first. Update tests
Attachment #8568442 -
Flags: review?(cajbir.bugzilla)
Comment 37•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Attachment #8568442 -
Flags: review?(cajbir.bugzilla) → review+
Assignee | ||
Comment 38•10 years ago
|
||
Comment on attachment 8568442 [details] [diff] [review]
Make end argument an unrestricted double as per spec
Bobby, I need the WebIDL part reviewed by a "DOM peer" and since you appear to not just be doing media anymore :)
Attachment #8568442 -
Flags: review?(bobbyholley)
Comment 39•10 years ago
|
||
Comment on attachment 8568442 [details] [diff] [review]
Make end argument an unrestricted double as per spec
Review of attachment 8568442 [details] [diff] [review]:
-----------------------------------------------------------------
r=me on the webidl change.
Attachment #8568442 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 40•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8e7a594541
(the regex checking for DOM peer needs to be fixed, it must have a space before r=)
Comment 41•10 years ago
|
||
Comment 43•10 years ago
|
||
Comment on attachment 8567711 [details] [diff] [review]
Add trimming support before playback position
Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance. Possible stalls playing MSE video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Implements missing MSE functionality. No regression risk outside that feature.
[String/UUID change made/needed]: None.
Flags: needinfo?(giles)
Attachment #8567711 -
Flags: approval-mozilla-beta?
Attachment #8567711 -
Flags: approval-mozilla-aurora?
Comment 44•10 years ago
|
||
Comment on attachment 8568442 [details] [diff] [review]
Make end argument an unrestricted double as per spec
Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance, less consistent testing.
[Describe test coverage new/current, TreeHerder]: Landed on m-c. We pass more web platform tests with this patch.
[Risks and why]: Small MSE-specific change, so low.
[String/UUID change made/needed]: None.
Attachment #8568442 -
Flags: approval-mozilla-beta?
Attachment #8568442 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8567711 -
Flags: approval-mozilla-beta?
Attachment #8567711 -
Flags: approval-mozilla-beta+
Attachment #8567711 -
Flags: approval-mozilla-aurora?
Attachment #8567711 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8568442 -
Flags: approval-mozilla-beta?
Attachment #8568442 -
Flags: approval-mozilla-beta+
Attachment #8568442 -
Flags: approval-mozilla-aurora?
Attachment #8568442 -
Flags: approval-mozilla-aurora+
Comment 45•10 years ago
|
||
Reset the status for these two new patch, fyi Ryan.
Updated•10 years ago
|
Comment 46•10 years ago
|
||
Updated•10 years ago
|
Flags: needinfo?(giles)
Comment 48•10 years ago
|
||
Comment 49•10 years ago
|
||
Please open a follow-up bug for any remaining issues here.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(jyavenard)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•