I wrote these patches as incremental steps to improving the keyframe logic. It sounds like jya is going to be refatoring a bunch of this stuff soon, but I think these patches will probably make his life easier, so let's get them landed.

Green try push for this here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6e8459865f3

Comment on attachment 8597336 [details] [diff] [review] Part 1 - Store timestamps on the NesteggPacketHolder. v1 Review of attachment 8597336 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webm/WebMReader.h @@ +42,5 @@ > + } > + > + // We store the timestamp as signed microseconds so that it's easily > + // comparable to other timestamps we have in the system. > + mTimestamp = timestamp_ns / 1000; s/1000/NS_PER_USEC/ @@ +49,5 @@ > + > + return true; > + } > + > + bool IsInitialized() { return mOffset >= 0; } This can be private.

Comment on attachment 8597337 [details] [diff] [review] Part 2 - Use NesteggPacketHolder::Timestamp to simplify code. v1 Review of attachment 8597337 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webm/IntelWebMVideoDecoder.cpp @@ +187,4 @@ > mReader->PushVideoPacket(next_holder.forget()); > } else { > next_tstamp = tstamp; > next_tstamp += tstamp - mReader->GetLastVideoFrameTime(); GetLastVideoFrameTime, SetLastVideoFrameTime, mLastVideoFrameTime need to be updated to be int64_t.

(In reply to Matthew Gregan [:kinetik] from comment #6) > s/1000/NS_PER_USEC/ That requires re-ordering some things and putting NesteggPacketHolder into namespace mozilla, but looks like that should be fine.

For reference, a green try push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6e8459865f3

Comment on attachment 8597340 [details] [diff] [review] Part 4 - Eagerly compute keyframe-ness and stash it on the packet holder. v1 Review of attachment 8597340 [details] [diff] [review]: ----------------------------------------------------------------- r+ as-is, because this isn't technically incorrect, but having researched further we can make this simpler, see comment below. ::: dom/media/webm/WebMReader.cpp @@ +930,5 @@ > + // abstraction, but timestamps are on the packet, so the only time > + // we have multiple video frames in a packet is when we have "alternate > + // reference frames", which are a compression detail and never displayed. > + // So for our purposes, we can just take the union of the is_kf values for > + // all the frames in the packet. I put you wrong when we discussed this the other day. Having done a bit more digging, I've verified that the reference encoder will produce alternate reference frames in a standalone packet (Block) with its own timestamp. So I think we should simplify all of this code by assuming that, for packets originating from video tracks, there is only one frame per packet.

(In reply to Matthew Gregan [:kinetik] from comment #10) > Comment on attachment 8597340 [details] [diff] [review] > Part 4 - Eagerly compute keyframe-ness and stash it on the packet holder. v1 > > Review of attachment 8597340 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ as-is, because this isn't technically incorrect, but having researched > further we can make this simpler, see comment below. > > ::: dom/media/webm/WebMReader.cpp > @@ +930,5 @@ > > + // abstraction, but timestamps are on the packet, so the only time > > + // we have multiple video frames in a packet is when we have "alternate > > + // reference frames", which are a compression detail and never displayed. > > + // So for our purposes, we can just take the union of the is_kf values for > > + // all the frames in the packet. > > I put you wrong when we discussed this the other day. Having done a bit > more digging, I've verified that the reference encoder will produce > alternate reference frames in a standalone packet (Block) with its own > timestamp. So I think we should simplify all of this code by assuming that, > for packets originating from video tracks, there is only one frame per > packet. Given that this is all ready to go and green on try, I'm going to file a followup bug on that. Thanks for the reviews! remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/978b0178407f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4cdacd756a45 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/20293705007c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cac2e68dd217

https://hg.mozilla.org/mozilla-central/rev/978b0178407f https://hg.mozilla.org/mozilla-central/rev/4cdacd756a45 https://hg.mozilla.org/mozilla-central/rev/20293705007c https://hg.mozilla.org/mozilla-central/rev/cac2e68dd217