1400598 - [MSE] MP4 bytestream is parsed several times unecessarily

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Jean-Yves Avenard [:jya]]

In order to check the validity of a stream, we parse all atom types.

However we do so on the entire content, once to determine if we have an init segment, once to determine if we have a moof and once again to check for a moof+mdat

If the streams contains several moof+mdat we will scan the entire stream again for as many times we have moof.

This is very inefficient. We could only parse the adata that we actually need at the time.

That is, if all we need to check is the init segment, that we can stop checking the content as soon as we have seen the init segment.

The spec does indicate that: "If the input buffer contains bytes that violate the SourceBuffer byte stream format specification, then run the append error algorithm and abort this algorithm."
(https://w3c.github.io/media-source/#sourcebuffer-segment-parser-loop)

so we should perform the check on the entire stream before check for the specific part.

An alternative would be to check each segment only as we go, considering any errors are fatal...

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1400598 - P1. Stop parsing bytesteam as soon as we have found the necessary data.

Review commit: https://reviewboard.mozilla.org/r/180636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180636/

Comment 2

[Jean-Yves Avenard [:jya]]

With the change, running firefox for bug 1400254 under rr goes from over 5 minutes to less than 20s.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8909043 [details]
Bug 1400598 - P1. Stop parsing bytesteam as soon as we have found the necessary data.

https://reviewboard.mozilla.org/r/180636/#review185744

::: dom/media/mediasource/ContainerParser.cpp:475
(Diff revision 1)
>    public:
> -    AtomParser(const MediaContainerType& aType, const MediaByteBuffer* aData)
> +    enum class StopAt
> +    {
> +      eInitSegment,
> +      eMediaSegment,
> +      eAll

Nitpick: 'eAll' could ambiguously be thought to mean "all nodes can be stopped at". How about "eEnd" instead?

::: dom/media/mediasource/ContainerParser.cpp:545
(Diff revision 1)
> +        if (aStop == StopAt::eInitSegment && (mInitOffset || mMediaOffset)) {
> +          break;

We want to stop at an init segment, but we would also stop at a media, is that intended?
(Is that because there should always be an init before a media, so it's an error and there's no point continuing?)

::: dom/media/mediasource/ContainerParser.cpp:548
(Diff revision 1)
> +        if (aStop == StopAt::eMediaSegment &&
> +            (mInitOffset || (mMediaOffset && mDataOffset))) {

We want to stop at a media segment, but we'd also stop at an init?
If my previous assumption was correct (always init before media), don't you mean '&&' instead of '||' here?

Or, seeing the 'StartWith...' tests above, is it because we don't expect an init at all, so we should stop on this error anyway.

If I'm correct, this now makes me think that 'StopAt' may be a bit misleading, as it implies "keep reading anything until we find something specific".
How about "Expect", "ExpectOnly", or something like that?

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8909043 [details]
Bug 1400598 - P1. Stop parsing bytesteam as soon as we have found the necessary data.

https://reviewboard.mozilla.org/r/180636/#review185744

> We want to stop at an init segment, but we would also stop at a media, is that intended?
> (Is that because there should always be an init before a media, so it's an error and there's no point continuing?)

We only want to deal with an init segment, if that's the first in the stream... if we see a media segment, there's no point continuing further. The code check return parser.StartWithInitSegment() ? NS_OK : NS_ERROR_NOT_AVAILABLE;

So in fact, continuing to parse after finding a moof when we know the test after will always be negative is a waste of resources.

> We want to stop at a media segment, but we'd also stop at an init?
> If my previous assumption was correct (always init before media), don't you mean '&&' instead of '||' here?
> 
> Or, seeing the 'StartWith...' tests above, is it because we don't expect an init at all, so we should stop on this error anyway.
> 
> If I'm correct, this now makes me think that 'StopAt' may be a bit misleading, as it implies "keep reading anything until we find something specific".
> How about "Expect", "ExpectOnly", or something like that?

same as for the init segment.

This code is called from IsMediaSegmentPresent, which then calls return parser.StartWithMediaSegment() 

So if we see an init segment while searching for a media segment, we know the later test will always be negative. So we can stop right there.

The original intent of the code was to scan the entire stream once to find any invalid boxes. But it evolved into much more than that.

If there's an invalid box, we will find it, but only after processing all the good data.

I think the bad data is rare enough that we should privilege the good data processing first...

So the "spirit" of the spec is preserved here.

I had felt that this would cause a problem for the reviewer, i should have put better comments. it's not obvious at first

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8909043 [details]
Bug 1400598 - P1. Stop parsing bytesteam as soon as we have found the necessary data.

https://reviewboard.mozilla.org/r/180636/#review185744

> same as for the init segment.
> 
> This code is called from IsMediaSegmentPresent, which then calls return parser.StartWithMediaSegment() 
> 
> So if we see an init segment while searching for a media segment, we know the later test will always be negative. So we can stop right there.
> 
> The original intent of the code was to scan the entire stream once to find any invalid boxes. But it evolved into much more than that.
> 
> If there's an invalid box, we will find it, but only after processing all the good data.
> 
> I think the bad data is rare enough that we should privilege the good data processing first...
> 
> So the "spirit" of the spec is preserved here.
> 
> I had felt that this would cause a problem for the reviewer, i should have put better comments. it's not obvious at first

the other change now is that we stop scanning only *after* reading the entire box, while before we would have stopped as soon as the box type was read.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8909043 [details]
Bug 1400598 - P1. Stop parsing bytesteam as soon as we have found the necessary data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180636/diff/1-2/

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8909043 [details]
Bug 1400598 - P1. Stop parsing bytesteam as soon as we have found the necessary data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180636/diff/2-3/

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/15e338118aa2