1158226 - Use NesteggPacketHolder to simplify code

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Store timestamps on the NesteggPacketHolder. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use NesteggPacketHolder::Timestamp to simplify code. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Separate packet demuxing from the higher-level track-sorting logic. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Eagerly compute keyframe-ness and stash it on the packet holder. v1

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Matthew Gregan [:kinetik]]

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 7

[Matthew Gregan [:kinetik]]

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.

Comment 8

[Bobby Holley (:bholley)]

(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.

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Matthew Gregan [:kinetik]]

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.

Comment 11

[Bobby Holley (:bholley)]

(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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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