Bug 184737 – [MSE] Add allSamplesInTrackEnqueued event

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Alicia Boya García]

MediaSource has a .endOfStream() method to signal when there are no more frames
after the ones currently buffered.

This bit of data is important for some multimedia frameworks. For instance, in
GStreamer a stream of frames being decoded should be terminated by a
'end-of-stream' (EOS) event that has a similar meaning. Some GStreamer elements
will expect this event in order to work properly under some circumstances.

Unfortunately currently WebKit provides no mechanism for this: an event of
sorts should be emitted after no more frames are going to be enqueued to signal
the end of the stream. The closest mechanism WebKit has for this is
`markEndOfStream()`, but it's not exactly the same: markEndOfStream() informs
that -- as far as network buffering is concerned -- we are done; but at that
point there may still be (and often are) many frames waiting in the
decodeQueue, so it would be wrong to signal the decoder that there are no more
frames.

This patch introduces a new optional method in SourceBufferPrivate,
`allSamplesInTrackEnqueued(const AtomicString& trackID)` that is called
whenever the MediaSource is in "ended" state (the user has called
`MediaSource.endOfStream()`) and the decodeQueue is empty. Media framework
implementations can use this method to send a EOS event to a decoder that needs
it.

[Alicia Boya García]

Created attachment 338229 [details]
Patch

[Jer Noble]

Changes to SourceBuffer and MediaSource LGTM, but probably needs a GTK reviewer for the GTK parts.

[WebKit Commit Bot]

Comment on attachment 338229 [details]
Patch

Clearing flags on attachment: 338229

Committed r230909: <https://trac.webkit.org/changeset/230909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39642725>

[Jer Noble]

Looks like this broke some 32-bit builds due to a mismatch between trackBuffer.decodeQueue.size() (a size_t) and the printf %lu directive. This should probably be a %zu instead. Don't know why the mac-32bit EWS bot didn't catch this though.

[Alicia Boya García]

(In reply to Jer Noble from comment #6)
> Looks like this broke some 32-bit builds due to a mismatch between
> trackBuffer.decodeQueue.size() (a size_t) and the printf %lu directive. This
> should probably be a %zu instead. Don't know why the mac-32bit EWS bot
> didn't catch this though.

Good catch.

[Daniel Bates]

Ryan Haddad attempted a build fix in <https://trac.webkit.org/changeset/230922>. As it turned out this caused an compile-time failure on an Apple Internal debug builder that treated size_type (the return value of std::map::size()) as equivalent to unsigned int.

[Daniel Bates]

Cast std::map::size() to size_t before logging and committed in <https://trac.webkit.org/changeset/230951>.

[Xabier Rodríguez Calvar]

(In reply to Jer Noble from comment #6)
> Looks like this broke some 32-bit builds due to a mismatch between
> trackBuffer.decodeQueue.size() (a size_t) and the printf %lu directive. This
> should probably be a %zu instead. Don't know why the mac-32bit EWS bot
> didn't catch this though.

Maybe because LOG calls are compiled out?

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #10)
> (In reply to Jer Noble from comment #6)
> > Looks like this broke some 32-bit builds due to a mismatch between
> > trackBuffer.decodeQueue.size() (a size_t) and the printf %lu directive. This
> > should probably be a %zu instead. Don't know why the mac-32bit EWS bot
> > didn't catch this though.
> 
> Maybe because LOG calls are compiled out?

And in the console I don't see any Apple debug build for 32bits.

[Michael Catanzaro]

"""According to <​http://en.cppreference.com/w/cpp/container/map> size_type is "usually a size_t", but it may not be."""

Because programming is more fun when types are uncertain!

[Daniel Bates]

(In reply to Michael Catanzaro from comment #12)
> """According to <​http://en.cppreference.com/w/cpp/container/map> size_type
> is "usually a size_t", but it may not be."""
> 
> Because programming is more fun when types are uncertain!

:)

CC'ing JF Bastien. Maybe he has some insight in the ambiguousness or can help propose a change to remove this ambiguousness.

[JF Bastien]

I'm not sure I understand: casting to size_t and using %zu didn't work?

[Alicia Boya García]

(In reply to JF Bastien from comment #14)
> I'm not sure I understand: casting to size_t and using %zu didn't work?

Casting surely works, what is surprising is that std::map::size_type is not guaranteed to be a std::size_t.

[JF Bastien]

(In reply to Alicia Boya García from comment #15)
> (In reply to JF Bastien from comment #14)
> > I'm not sure I understand: casting to size_t and using %zu didn't work?
> 
> Casting surely works, what is surprising is that std::map::size_type is not
> guaranteed to be a std::size_t.

Ah OK gotcha!

To answer your question: the Committee works in mysterious ways 🤡

More seriously: I have no clue. I dug in the standard and see no obvious reason. I'm asking committee folks, and might write a paper to Make It So.

[JF Bastien]

(In reply to JF Bastien from comment #16)
> (In reply to Alicia Boya García from comment #15)
> > (In reply to JF Bastien from comment #14)
> > > I'm not sure I understand: casting to size_t and using %zu didn't work?
> > 
> > Casting surely works, what is surprising is that std::map::size_type is not
> > guaranteed to be a std::size_t.
> 
> Ah OK gotcha!
> 
> To answer your question: the Committee works in mysterious ways 🤡
> 
> More seriously: I have no clue. I dug in the standard and see no obvious
> reason. I'm asking committee folks, and might write a paper to Make It So.

I asked and it sounds like there's not a good reason to not have this guarantee (size_type == size_t) for a container which uses the *default* allocator. It makes sense to let users customize size_type with their own allocator (say on 64-bit you want smaller vector, so you restrict to 2GiB of 4GiB), but that's not your case.

It'll likely be an ABI break for some implementations, but I think there are workarounds for them. I can probably get it fixed for C++20 if you think it's worth going for.