Closed Bug 1261900 Opened 9 years ago Closed 8 years ago

WebM using MSE does not play properly with BlockDuration

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Version:
45 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox48 + wontfix
firefox49 + verified

People

(Reporter: modmaker, Assigned: kinetik)

References

Details

Attachments

(9 files, 3 obsolete files)

MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik
58 bytes, text/x-review-board-request
kinetik
: review+
Details
MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik
58 bytes, text/x-review-board-request
kinetik
: review+
Details
MozReview Request: Bug 1261900: [webm] P9. Prevent null deref when webm logs are turned on. r?kinetik
58 bytes, text/x-review-board-request
kinetik
: review+
Details
replacement p7
44 bytes, text/x-github-pull-request
rillian
: review+
Details | Review
replacement p8
1.53 KB, patch
jya
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36 Steps to reproduce: Play the video below using MSE. The last frame in each Cluster is a BlockGroup that contains the BlockDuration and a ReferenceBlock. Navigate to the player link below. Click 'Load Stream' and the video should load and play properly. Below are the media links for the DASH manifest and the original WebM media. Example Player: http://shaka-player-demo.appspot.com/?asset=https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-webm-only/dash.mpd DASH Manifest: https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-webm-only/dash.mpd WebM Video: https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-webm-only/v-0576p-1000k-vp9.webm Actual results: The first segment is buffered correctly and plays. However future segments do not report error, but do not extend the duration. It is not possible to seek to these regions because it is considered unbuffered. This also does not produce multiple buffered ranges. The player will buffer all of the segments and once it has it will call EndOfStream. Because each append does not change the duration, the stream will be reduced to the 12 seconds that can play, even though the stream is 1 minute long. Expected results: The segments should continue to append successfully and change the duration. The video should play for 1 minute. Playing on Chrome will correctly play for 1 minute. So does playing the video directly (i.e. not using MSE).
Assignee: nobody → jyavenard
kinetik the issue appears to be how nestegg demux. The WebMBufferedParser properly identify all the clusters and the start and end time for each cluster along the offset position of each. From this we create a MediaResource with just the init segment (byte 0 to 285) followed by the first cluster. The first cluster is identified to go from 285 to 712421 (712136 bytes long) We then use the WebMDemuxer to demux all samples. when reading the last packet, nestegg_read_packet returns 0 and set ctx->ancestor to NULL. We then add to the MediaResource the second cluster (from position 712421 and is 712127 bytes long), and against start to demux. however nestegg_read_packet returns -1 after starting second cluster because ctx->ancestor is not set. :kinetik, could you have a look and see what's wrong with this? thank you.
Flags: needinfo?(kinetik)
Priority: -- → P1
This allows to resume demuxing once more data is added to the resource. Review commit: https://reviewboard.mozilla.org/r/44255/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44255/
Attachment #8738023 - Flags: review?(kinetik)
It's kind of unfortunate that we rely on the behaviour you describe where libnestegg sees an EOS but then is expected to continue parsing later as it's not the intended use of the API; once nestegg_read_packet returns 0 you're supposed to finish using that libnestegg context. Bug 1257699 introduced the ctx->ancestor check to make the EOS handling more robust, before that ne_parse or ne_peek_element would just return EOS anyway - reverting that change doesn't seem to make the test behave any differently, anyway. To make this work as-is, perhaps there needs to be a way for the io_read callback to signal that there's no more data available but EOS has not been reached, but the caller would have to promise this only happened at the end of a Cluster to ensure the parser could be resumed safely as libnestegg is not desired to resume parsing from arbitrary offsets.
Flags: needinfo?(kinetik)
I had a quick shot at it, feel free to dismiss it entirely if you have a better approach. The issue is that ne_parse always attempt to peek at the next element, and consider EOS as final (it will put the context in a way that can't be resumed). I added a quick EOS detection so that if we hit EOS just as we attempt to read a new element id we abort early and leave the context as-is so that when new data is added to the resource and we attempt to demux it can resume nicely.
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/1-2/
sorry for the conflict in comments. I had narrowed the EOS detection so actual error aren't treated as eos.
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik https://reviewboard.mozilla.org/r/44255/#review40933 I don't think this is the approach I want to take dealing with this, but I haven't had time to think through the changes I would make to explain them fully here. The rough idea is that the io_read callback would grow a new way to signal the no-data but not-EOS case. Handling it at the point of trying to read the next element ID is on the right track, but it probably needs to only happen at points libnestegg expects to pause and resume parsing (aka "suspend" elements), where the caller is promising that the next element that will occur after signalling no-more-data would be either a (Simple)Block or a new Cluster and anything else would be treated as a stream error.
Attachment #8738023 - Flags: review?(kinetik)
Per IRC, bug 1257699 will likely regress current webm/MSE when dealing with partial media segment. as not only would we hit EOS, but we would hit EOS on a possibly partial element.
Blocks: 1257699
Assignee: jyavenard → kinetik
(In reply to Matthew Gregan [:kinetik] from comment #8) > trying to read the next element ID is on the right track, but it probably > needs to only happen at points libnestegg expects to pause and resume > parsing (aka "suspend" elements), where the caller is promising that the > next element that will occur after signalling no-more-data would be either a Upon reflection, this is not something we can promise. It is possible the MSE only receive a webm file, truncated at random. So where EOS occurs is never defined. Test 30, of YouTube compliance test, with webm on. https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?timestamp=1461237361297 will append media segment truncated at random (and just had a crash, which will be fixed in my P2). YouTube also when seeking will append a media segment with about 1s. A typical youtube media segment is 5s long, it is my guess that to get about 1s worth, they simply look at the size and divide by 5. As such, we must be able to resume anywhere within the file when demuxing. An alternative, but that's not really pretty, would be that every time a new segment is appended, we would drop all frames currently demuxed and stored in the track buffer. Re-create a demuxer and redemux it all. The only other demuxer MSE can deal with is the mp4 one, which can resume anywhere. So my preference at this stage is to make nestegg able to handle this case. The downsides of clearing the trackbuffer each time being pretty awful.
Encountered while using YouTube MSE/webm compliance test. Review commit: https://reviewboard.mozilla.org/r/48611/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48611/
Attachment #8738023 - Attachment description: MozReview Request: Bug 1261900: [nestegg] Don't treat EOS as error when reaching end of valid element. r?kinetik → MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik
Attachment #8744553 - Flags: review?(gsquelart)
Attachment #8744554 - Flags: review?(gsquelart)
Attachment #8744555 - Flags: review?(gsquelart)
Attachment #8744556 - Flags: review?(gsquelart)
Attachment #8744557 - Flags: review?(kinetik)
Attachment #8744558 - Flags: review?(kinetik)
Attachment #8744559 - Flags: review?(kinetik)
Attachment #8738023 - Flags: review?(kinetik)
MSE only uses the webm demuxer to demux all samples at once. Attempting to find the next keyframe as such always fail. Review commit: https://reviewboard.mozilla.org/r/48619/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48619/
This commit is just for review purposes, the changes will go upstream. Review commit: https://reviewboard.mozilla.org/r/48621/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48621/
Our nestegg demuxer is not designed to continue demuxing once it has hit an error or EOS. However, being able to resume demuxing is a fundamental requirement of the MSE architecture as it is possible to receive partial media segments (webm cluster) at any time or a complete media segment but incomplete webm file. We get around this limitation by saving the nestegg memory context prior demuxing and detecting when nestegg hit EOS. If so, we restore the saved memory context so that nestegg can resume; in effect it's as if nestegg didn't hit EOS. Review commit: https://reviewboard.mozilla.org/r/48623/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48623/
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/2-3/
Comment on attachment 8744553 [details] MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald https://reviewboard.mozilla.org/r/48611/#review45361 r+ with typo fixed in check-in comment: "should first media segment contained" -> "should first media segment contain" or "if first media segment contains".
Attachment #8744553 - Flags: review?(gsquelart) → review+
Comment on attachment 8744554 [details] MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald https://reviewboard.mozilla.org/r/48613/#review45363 ::: dom/media/MediaDataDemuxer.h:206 (Diff revision 1) > > virtual media::TimeIntervals GetBuffered() = 0; > > + // By default, it is assumed that the entire resource can be evicted once > + // all samples have been demuxed. > + virtual int64_t GetEvictionOffset(const media::TimeUnit& aTime) Could this method be const? (Might not be possible if some overrides may modify their object, like the old MP4TrackDemuxer::GetEvictionOffset from before bug 1204419.)
Attachment #8744554 - Flags: review?(gsquelart) → review+
Attachment #8744555 - Flags: review?(gsquelart) → review+
Comment on attachment 8744555 [details] MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald https://reviewboard.mozilla.org/r/48615/#review45365
bug 1265043 is widevine, which doesn't support webm so can't be related to this bug.
No longer blocks: 1265043
thank you for the reviews ! I used the old code as-is. We could make the webm GetEvictionOffset const, though the internal GetOffsetForTime isn't const and use a reentrant monitor (which could be removed now though)
Attachment #8744556 - Flags: review?(gsquelart) → review+
Comment on attachment 8744556 [details] MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald https://reviewboard.mozilla.org/r/48617/#review45367 ::: dom/media/webm/WebMDemuxer.cpp:1038 (Diff revision 1) > > +int64_t > +WebMTrackDemuxer::GetEvictionOffset(const media::TimeUnit& aTime) > +{ > + int64_t offset; > + if (!mParent->GetOffsetForTime(aTime.ToNanoseconds(), &offset)) { I'll just have to trust you about giving nanoseconds to GetOffsetForTime -- or you should find a reviewer who knows this. Rant: I find it difficult to know which units are used around WebMDemuxer (e.g. just looking at one pair of member vars: mCodecDelay is in microseconds, but mSeekPreroll is in nanoseconds; Other things seem to use time scales). I would really encourage trying to name variables&parameters with units where possible (e.g. mCodecDelay_ms, mSeekPreroll_ns, aTime_ns?), or even better: use different types or an auto-converting type for different time units, to statically catch potential errors.
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/3-4/
Attachment #8744553 - Attachment description: MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion should first media segment contained no frame. r?gerald → MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald
Attachment #8744554 - Attachment description: MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r?gerald → MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald
Attachment #8744555 - Attachment description: MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r?gerald → MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald
Attachment #8744556 - Attachment description: MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r?gerald → MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald
Comment on attachment 8744553 [details] MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48611/diff/1-2/
Comment on attachment 8744554 [details] MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48613/diff/1-2/
Comment on attachment 8744555 [details] MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48615/diff/1-2/
Comment on attachment 8744556 [details] MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48617/diff/1-2/
Comment on attachment 8744557 [details] MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48619/diff/1-2/
Comment on attachment 8744558 [details] MozReview Request: Bug 1261900: Update in-tree libnestegg. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48621/diff/1-2/
Comment on attachment 8744559 [details] MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48623/diff/1-2/
(In reply to Gerald Squelart [:gerald] from comment #24) > Comment on attachment 8744556 [details] > MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add > WebMTrackDemuxer::GetEvictionOffset. r=gerald > > https://reviewboard.mozilla.org/r/48617/#review45367 > > ::: dom/media/webm/WebMDemuxer.cpp:1038 > (Diff revision 1) > > > > +int64_t > > +WebMTrackDemuxer::GetEvictionOffset(const media::TimeUnit& aTime) > > +{ > > + int64_t offset; > > + if (!mParent->GetOffsetForTime(aTime.ToNanoseconds(), &offset)) { > > I'll just have to trust you about giving nanoseconds to GetOffsetForTime -- > or you should find a reviewer who knows this. This is not new code. I just reverted the commits from bug 1204419. Which is why I got you to review. The WebM container store all timing information in nanoseconds (http://www.webmproject.org/docs/container/) and so most timing out there uses nanoseconds too. While everything is based on timescales, the timescale itself is in nanoseconds That code got almost transferred straight from the WebMReader ; need to update to TimeUnit thorough the media code (there's a bug for that opened by Dan) mCodecDelay is something used by the Opus codec, which expects microseconds. So it's stored in the webm in nanoseconds, and passed to opus in microseconds. simply no? :)
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik https://reviewboard.mozilla.org/r/44255/#review45623 We really need to merge the blockgroup nestegg branch and land that in Gecko before nestegg_packet_duration will work reliably. ::: dom/media/webm/WebMDemuxer.cpp:526 (Diff revision 4) > if (aType == TrackInfo::kAudioTrack) { > RefPtr<NesteggPacketHolder> next_holder(NextPacket(aType)); > if (next_holder) { > next_tstamp = next_holder->Timestamp(); > PushAudioPacket(next_holder); > + } else if (duration > 0) { The block duration could be zero, theoretically, in which case we'd estimate an incorrect duration here. ::: dom/media/webm/WebMDemuxer.cpp:541 (Diff revision 4) > } else if (aType == TrackInfo::kVideoTrack) { > RefPtr<NesteggPacketHolder> next_holder(NextPacket(aType)); > if (next_holder) { > next_tstamp = next_holder->Timestamp(); > PushVideoPacket(next_holder); > + } else if (duration > 0) { Ditto.
Attachment #8738023 - Flags: review?(kinetik) → review+
Comment on attachment 8744557 [details] MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik https://reviewboard.mozilla.org/r/48619/#review45625
Attachment #8744557 - Flags: review?(kinetik) → review+
https://reviewboard.mozilla.org/r/44255/#review45623 > The block duration could be zero, theoretically, in which case we'd estimate an incorrect duration here. Right now, nestegg is almost always reporting 0. But having said that, when the block duration is 0, it just keep using the previous behavior; that is either estimate the duration based on the previous duration (plain webm) or wait until the next block to know the start time (MSE) except if there won't be future data, in which case it use the previous duration. So if the duration is really 0, then the next block would have the same starting time as the current block. So it's all good.
Assignee: kinetik → jyavenard
Depends on: 1267513
(In reply to Jean-Yves Avenard [:jya] from comment #36) > https://reviewboard.mozilla.org/r/44255/#review45623 > > > The block duration could be zero, theoretically, in which case we'd estimate an incorrect duration here. > > Right now, nestegg is almost always reporting 0. Yes, because of my first comment in comment 34. > But having said that, when > the block duration is 0, it just keep using the previous behavior; that is > either estimate the duration based on the previous duration (plain webm) or > wait until the next block to know the start time (MSE) except if there won't > be future data, in which case it use the previous duration. > > So if the duration is really 0, then the next block would have the same > starting time as the current block. So it's all good. Except if there's no next block, in which case it'll use the estimate and produce the wrong value.
Comment on attachment 8744559 [details] MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik https://reviewboard.mozilla.org/r/48623/#review45711 ::: dom/media/webm/WebMDemuxer.h:221 (Diff revision 2) > Maybe<uint32_t> mLastSeenFrameHeight; > // This will be populated only if a resolution change occurs, otherwise it > // will be left as null so the original metadata is used > RefPtr<SharedTrackInfo> mSharedVideoTrackInfo; > + > + // Set to true if nestegg read's callback hit EOS. No "'s", I think ::: dom/media/webm/WebMDemuxer.cpp:677 (Diff revision 2) > RefPtr<NesteggPacketHolder> > WebMDemuxer::DemuxPacket() > { > + // Wrapper class that will save the current nestegg context and restore it > + // if necessary or discard the the state backup upon destruction. > + class NestEggState { Annotate with MOZ_RAII. Doesn't matter much right now, since it's single use.
Attachment #8744559 - Flags: review?(kinetik) → review+
Comment on attachment 8744558 [details] MozReview Request: Bug 1261900: Update in-tree libnestegg. r?kinetik https://reviewboard.mozilla.org/r/48621/#review45721 Needs to be submitted upstream and pulled back in via media/libnestegg/update.sh. ::: media/libnestegg/src/nestegg.c:179 (Diff revision 2) > char * s; > struct ebml_binary b; > } v; > enum ebml_type_enum type; > int read; > + int64_t position; Call it offset ::: media/libnestegg/src/nestegg.c:864 (Diff revision 2) > ctx->ancestor = item->previous; > free(item); > } > > static int > -ne_ctx_save(nestegg * ctx, struct saved_state * s) > +ne_ctx_save(nestegg * ctx, struct nestegg_state * s) Drop "struct" here and in ne_ctx_restore now that nestegg_state is a typedef. ::: media/libnestegg/src/nestegg.c:998 (Diff revision 2) > if (storage->read) { > - ctx->log(ctx, NESTEGG_LOG_DEBUG, "element %llx (%s) already read, skipping", > + ctx->log(ctx, NESTEGG_LOG_DEBUG, "element %llx (%s) already read", > desc->id, desc->name); > + /* We do not need to re-read the element, however we do need to move the IO > + position back to the original offset */ > + if (storage->position >= 0) { I'm not sure this is the right place to handle this; it feels a bit awkward to hide the IO stream offset save/restore down in here. I think I'd prefer it hoisted up to the main parser loop, but I don't think it's easy to do that in a nice way right now... so maybe this is okay for now. ::: media/libnestegg/src/nestegg.c:1721 (Diff revision 2) > { > int r; > struct ebml_list_node * node = ctx->segment.cues.cue_point.head; > struct seek * found; > uint64_t seek_pos, id; > - struct saved_state state; > + struct nestegg_state state; No "struct" here too ::: media/libnestegg/src/nestegg.c:1997 (Diff revision 2) > free(ctx->io); > free(ctx); > } > > int > +nestegg_save_state(nestegg * ctx, struct nestegg_state ** state) Ditto ::: media/libnestegg/src/nestegg.c:2026 (Diff revision 2) > + *state = s; > + return 0; > +} > + > +void > +nestegg_restore_state(nestegg * ctx, struct nestegg_state * s) Ditto
Attachment #8744558 - Flags: review?(kinetik) → review+
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/4-5/
Attachment #8744558 - Attachment description: MozReview Request: Bug 1261900: [nestegg] P7. Add nestegg methods to save/restore context. r?kinetik → MozReview Request: Bug 1261900: Update in-tree libnestegg. r?kinetik
Comment on attachment 8744553 [details] MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48611/diff/2-3/
Comment on attachment 8744554 [details] MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48613/diff/2-3/
Comment on attachment 8744555 [details] MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48615/diff/2-3/
Comment on attachment 8744556 [details] MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48617/diff/2-3/
Comment on attachment 8744557 [details] MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48619/diff/2-3/
Comment on attachment 8744558 [details] MozReview Request: Bug 1261900: Update in-tree libnestegg. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48621/diff/2-3/
Comment on attachment 8744559 [details] MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48623/diff/2-3/
https://reviewboard.mozilla.org/r/48621/#review45721 > I'm not sure this is the right place to handle this; it feels a bit awkward to hide the IO stream offset save/restore down in here. I think I'd prefer it hoisted up to the main parser loop, but I don't think it's easy to do that in a nice way right now... so maybe this is okay for now. Yes. I believe the best method would be to drop this part of the code, and instead, in the restore_state parse the tree of objects and clear the read flag of all objects located after the stored context stream_offset. I have to identify however, why is it following the restore, we enter ne_read_simple with IO::offset 1 byte past the earlier position.
Following IRC discussion, also remove the temporary leak of already read ebml_binary or string if any. Review commit: https://reviewboard.mozilla.org/r/49113/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49113/
Comment on attachment 8738023 [details] MozReview Request: Bug 1261900: [webm] P1. Use block duration if known. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44255/diff/5-6/
Comment on attachment 8744553 [details] MozReview Request: Bug 1261900: [MSE] P2. Prevent assertion if first media segment contains no usable frames. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48611/diff/3-4/
Comment on attachment 8744554 [details] MozReview Request: Bug 1261900: P3. Re-add MediaDataDemuxer::GetEvictionOffset() API. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48613/diff/3-4/
Comment on attachment 8744555 [details] MozReview Request: Bug 1261900: [MSE] P4. Only evict no longer used data from resource. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48615/diff/3-4/
Comment on attachment 8744556 [details] MozReview Request: Bug 1261900: [MSE/webm] P5. Re-add WebMTrackDemuxer::GetEvictionOffset. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48617/diff/3-4/
Comment on attachment 8744557 [details] MozReview Request: Bug 1261900: [MSE/webm] P6. Don't unnecessarily calculate the next keyframe time. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48619/diff/3-4/
Comment on attachment 8744558 [details] MozReview Request: Bug 1261900: Update in-tree libnestegg. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48621/diff/3-4/
Comment on attachment 8744559 [details] MozReview Request: Bug 1261900: [MSE/webm] P8. Allow WebMDemuxer to resume demuxing even after encountering EOS. r?kinetik Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48623/diff/3-4/
Comment on attachment 8745819 [details] MozReview Request: Bug 1261900: [webm] P9. Prevent null deref when webm logs are turned on. r?kinetik https://reviewboard.mozilla.org/r/49111/#review45919 ::: dom/media/webm/WebMDemuxer.cpp:1048 (Diff revision 1) > WebMTrackDemuxer::SkipToNextRandomAccessPoint(media::TimeUnit aTimeThreshold) > { > uint32_t parsed = 0; > bool found = false; > RefPtr<MediaRawData> sample; > + int64_t sampleTime; Initialize sampleTime to avoid using an uninitialized value in the log in the case where sample used to be null.
Attachment #8745819 - Flags: review?(kinetik) → review+
https://reviewboard.mozilla.org/r/49111/#review45919 > Initialize sampleTime to avoid using an uninitialized value in the log in the case where sample used to be null. The value is only used if found is true. if so, the value is set but will initialise for clarity
Comment on attachment 8745820 [details] MozReview Request: Bug 1261900: [nestegg] P10. Improve handling of resume. r?kinetik https://reviewboard.mozilla.org/r/49113/#review45933 r+ with the h_free abstraction issue fixed ::: media/libnestegg/src/nestegg.c:1801 (Diff revision 1) > + case TYPE_BINARY: > + storage = (struct ebml_type *) (data + desc->offset); > + if (storage->offset >= stream_offset) { > + storage->read = 0; > + if (desc->type == TYPE_BINARY) { > + h_free(storage->v.b.data); Replace the h_frees with an ne_pool_free that takes the pointer and the pool pointer (which, with halloc, it won't use but some other impl might).
Attachment #8745820 - Flags: review?(kinetik) → review+
https://reviewboard.mozilla.org/r/49111/#review45919 > The value is only used if found is true. if so, the value is set but will initialise for clarity Oh right, missed that. Doesn't matter either way, then. Happy to leave it uninitialized.
[Tracking Requested - why for this release]: Change from bug 1257699 will cause regressions (typically stalls) with MSE/WEBM sites such as youtube. Need to ensure this goes into 48.
tracking-firefox48: --- → ?
As discussed on IRC, :kinetik will rework P7 and P8, P10 to be dropped
Assignee: jyavenard → kinetik
Tracking for 48 since the media team wants to uplift this to 48 once it lands. It would be best to land it (and test) on m-c first, and also, best to get this into aurora as soon as possible, since we usually don't want to do big multi-part uplifts right at the end of the release cycle.
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox48: ? → +
tracking-firefox49: --- → +
Flags: qe-verify+
Attachment #8745820 - Attachment is obsolete: true
Attachment #8744558 - Attachment is obsolete: true
Attachment #8744559 - Attachment is obsolete: true
Attached file replacement p7 —
Please see description in the PR.
Attachment #8753688 - Flags: review?(giles)
Attached patch replacement p8 — — Splinter Review
Use nestegg_read_reset to resume parsing at last valid point.
Attachment #8753690 - Flags: review?(jyavenard)
Comment on attachment 8753690 [details] [diff] [review] replacement p8 Review of attachment 8753690 [details] [diff] [review]: ----------------------------------------------------------------- I'm guessing you will change the commit log.
Attachment #8753690 - Flags: review?(jyavenard) → review+
Attachment #8753688 - Flags: review?(giles) → review+
Matthew, Jean-Yves, what are your plans wrt 48? The merge to beta will arrive soon.
Flags: needinfo?(kinetik)
Flags: needinfo?(jyavenard)
(In reply to Sylvestre Ledru [:sylvestre] from comment #75) > Matthew, Jean-Yves, what are your plans wrt 48? The merge to beta will > arrive soon. I reworked the fix from bug 1257699 in bug 1271866 to avoid the regression and landed that on aurora. That, at least, avoids making the problem worse than it was. I don't know if we'll want to uplift the complete fix from this bug as it's fairly large and thus carries some risk.
Flags: needinfo?(kinetik)
I think letting it ride the train is fine; if we really wanted to, uplifting to 48 will do if we really wanted to let shaka/google player work. As :kinetik mentioned, now that bug 1257699 got reverted there's no regression occurring.
Flags: needinfo?(jyavenard)
Based on comment 78, let it ride the train and won't fix in 48.
status-firefox48: affected → wontfix
Depends on: 1296988
I've managed to reproduce this issue on an older Nightly build 48.0a1 from 2016-04-04 using Windows 10 x64. This issue is verified fixed on Beta 49.0-build2 (2016-09-07)on the following OS's: - Windows 10 x64 - Ubuntu 16.04 LTS x64 - Mac OS X 10.10.5(Yosemite)
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: