1314533 - [MSE] appendbuffer should return an error when invalid data is being added

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Per spec:
https://w3c.github.io/media-source/index.html#sourcebuffer-segment-parser-loop

"If the input buffer contains bytes that violate the SourceBuffer byte stream format specification, then run the append error algorithm and abort this algorithm.
"

However, we do not do that. other than for the init segment, we ignore all errors and skip every invalid bytes until we find a media segment that starts properly.

Additionally, the various MediaDataDemuxer never return an error, they simply demux as much as they can. In case of error they always returns EOS.

Comment 1

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

One question around that:
Should we try and find all possible errors in streams, including for things we may not actually need?

E.g., in bug 1314441, I intend to remove more of libstagefright, including the parsing of '----' boxes, which contain some information we are currently parsing but then not actually using!
https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#2365
(And I wouldn't be surprised if we were already skipping some "uninteresting" boxes.)

So with that we could accept streams that, strictly speaking, "violate the SourceBuffer byte stream format specification", however we would still be capable of parsing and (probably) playing them.

Is that a good or bad thing?
I can think of arguments going both ways, most importantly: Play whenever possible (good for the user) vs strict compliance (bad for Mozilla / the Web, because we could play things that other compliant browsers rightly would refuse).

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: [MSE] P1. Change member prototype.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: [MSE] P2. Reject appendBuffer if invalid data found.

This still requires all the ContainerParser to be updated in properly handling errors.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: [MSE] P4. Reject invalid webm block.

An invalid webm block at this stage is either a non init segment or non media segment.

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

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: P5. Abort when webm parser encounter an error.

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

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: Do not expect init segment to be parsed when appending corrupted data.

From the Segment Parser Loop definition:
https://w3c.github.io/media-source/index.html#sourcebuffer-segment-parser-loop
"2. If the input buffer contains bytes that violate the SourceBuffer byte stream format specification, then run the append error algorithm and abort this algorithm."

The test appends data with a valid init segment followed by corrupted content.
The corrupted content satisfies the condition that the input buffer contains bytes that violate byte stream format specification.
As such, the append error algorithm is to be run prior parsing the init segment.

Add a new test verifying the defined behavior.

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

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1314533: [MSE] P8. Update wpt expected results.

We pass those tests now.

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

Comment 10

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

Comment on attachment 8807989 [details]
Bug 1314533: [MSE] P1. Change member prototype.

https://reviewboard.mozilla.org/r/90902/#review90606

::: dom/media/mediasource/ContainerParser.cpp:185
(Diff revision 1)
> -    if (mLastMapping && (initSegment || IsMediaSegmentPresent(aData))) {
> +    if (mLastMapping && (initSegment ||
> +        NS_SUCCEEDED(IsMediaSegmentPresent(aData)))) {

Style: &&/|| alignments are confusing. I think you should just move '(initSegment ||' to the following line, together with 'NS_SUCCEEDED...'.

Comment 11

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

Comment on attachment 8807990 [details]
Bug 1314533: [MSE] P2. Reject appendBuffer if invalid data found.

https://reviewboard.mozilla.org/r/90904/#review90608

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807989 [details]
Bug 1314533: [MSE] P1. Change member prototype.

https://reviewboard.mozilla.org/r/90902/#review90606

> Style: &&/|| alignments are confusing. I think you should just move '(initSegment ||' to the following line, together with 'NS_SUCCEEDED...'.

tell that to clang-format!

Comment 13

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

That's disappointing of clang. :-(

Comment 14

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

Comment on attachment 8807991 [details]
Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.

https://reviewboard.mozilla.org/r/90906/#review90610

::: dom/media/mediasource/ContainerParser.cpp:394
(Diff revision 1)
> -        uint32_t type = reader.ReadU32();
> +        mp4_demuxer::AtomType type = mp4_demuxer::AtomType(reader.ReadU32());
>          MSE_DEBUGV(AtomParser ,"Checking atom:'%c%c%c%c' @ %u",
>                     typec[0], typec[1], typec[2], typec[3],
>                     (uint32_t)reader.Offset() - 8);
> +
> +        for (uint32_t i = 0; i < ArrayLength(validBoxes); i++) {

Suggestion: Range-for should work here, e.g., 'for (const mp4_demuxer::AtomType& validBox : validBoxes)'.

::: dom/media/mediasource/ContainerParser.cpp:402
(Diff revision 1)
> +          mLastInvalidBox[0] = typec[3];
> +          mLastInvalidBox[1] = typec[2];
> +          mLastInvalidBox[2] = typec[1];
> +          mLastInvalidBox[3] = typec[0];

This assumes a big-endian system (I think). Would you be able to get the correct order in a platform-independent manner?
(Though in the end it is not *that* critical, as it's only a comment inside an error, so I'm fine with getting this right in another bug.)

::: dom/media/mediasource/ContainerParser.cpp:410
(Diff revision 1)
> -            mp4_demuxer::AtomType(type) == initAtom) {
> +              mp4_demuxer::AtomType(type) == initAtom) {
> -          mInitOffset = Some(reader.Offset());
> +            mInitOffset = Some(reader.Offset());
> -        }
> +          }

Typo? Remove added indentation.

Comment 15

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

Comment on attachment 8807992 [details]
Bug 1314533: [MSE] P4. Reject invalid webm block.

https://reviewboard.mozilla.org/r/90908/#review90614

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807991 [details]
Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.

https://reviewboard.mozilla.org/r/90906/#review90610

> This assumes a big-endian system (I think). Would you be able to get the correct order in a platform-independent manner?
> (Though in the end it is not *that* critical, as it's only a comment inside an error, so I'm fine with getting this right in another bug.)

box type *is* stored in big-endian. AFAIK, we only support little endian system.

Comment 17

[Matthew Gregan [:kinetik]]

Comment on attachment 8807993 [details]
Bug 1314533: P5. Abort when webm parser encounter an error.

https://reviewboard.mozilla.org/r/90910/#review90628

Comment 18

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

Comment on attachment 8807994 [details]
Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.

https://reviewboard.mozilla.org/r/90912/#review90624

r+ assuming the following questions don't raise any actual concerns.
In which case some extra comments in the code should help understand what will happen in the NOT_AVAILABLE case.

::: dom/media/mediasource/TrackBuffersManager.cpp:675
(Diff revision 1)
> +    if (!NS_SUCCEEDED(newData) && newData.Code() != NS_ERROR_NOT_AVAILABLE) {
> +      RejectAppend(newData, __func__);
> +      return;
> +    }
>      mProcessedInput += mInputBuffer->Length();

Can you claim to have processed the input in the NS_ERROR_NOT_AVAILABLE case?

Shouldn't you exit the parser loop with a NeedMoreData() in this case?

And will you restart processing this past data when the next buffer if appended?

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807994 [details]
Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.

https://reviewboard.mozilla.org/r/90912/#review90624

> Can you claim to have processed the input in the NS_ERROR_NOT_AVAILABLE case?
> 
> Shouldn't you exit the parser loop with a NeedMoreData() in this case?
> 
> And will you restart processing this past data when the next buffer if appended?

this is not what NS_ERROR_NOT_AVAILABLE means: it only indicates that the buffered parser hasn't found a new buffered range. This can happen with partial data, in which case the previous call would have already returned the full buffer range.
We only want to reject the data if an actual error was found.
We don't reject the appendBuffer if NS_ERROR_NOT_AVAILABLE is returned.

Comment 20

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

Comment on attachment 8807996 [details]
Bug 1314533: [MSE] P8. Update wpt expected results.

https://reviewboard.mozilla.org/r/90916/#review90632

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807989 [details]
Bug 1314533: [MSE] P1. Change member prototype.

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

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807990 [details]
Bug 1314533: [MSE] P2. Reject appendBuffer if invalid data found.

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

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807991 [details]
Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.

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

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807992 [details]
Bug 1314533: [MSE] P4. Reject invalid webm block.

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

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807993 [details]
Bug 1314533: P5. Abort when webm parser encounter an error.

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

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807994 [details]
Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.

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

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807995 [details]
Bug 1314533: Do not expect init segment to be parsed when appending corrupted data.

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

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807996 [details]
Bug 1314533: [MSE] P8. Update wpt expected results.

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807995 [details]
Bug 1314533: Do not expect init segment to be parsed when appending corrupted data.

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

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8807996 [details]
Bug 1314533: [MSE] P8. Update wpt expected results.

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

Comment 31

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

Comment on attachment 8807995 [details]
Bug 1314533: Do not expect init segment to be parsed when appending corrupted data.

https://reviewboard.mozilla.org/r/90914/#review90662

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d85e1b19808
[MSE] P1. Change member prototype. r=gerald
https://hg.mozilla.org/integration/autoland/rev/33c8e33b0bb0
[MSE] P2. Reject appendBuffer if invalid data found. r=gerald
https://hg.mozilla.org/integration/autoland/rev/754c160ecbed
[MSE] P3. Reject invalid MP4 boxes. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7c84e5453411
[MSE] P4. Reject invalid webm block. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ab4d4dfee074
P5. Abort when webm parser encounter an error. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/ca6e6783a379
[MSE] P6. Allow to detect error during preliminary container parsing. r=gerald
https://hg.mozilla.org/integration/autoland/rev/87882fa5ef49
Do not expect init segment to be parsed when appending corrupted data. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f26d5e27e1b6
[MSE] P8. Update wpt expected results. r=gerald

Comment 33

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

https://hg.mozilla.org/mozilla-central/rev/2d85e1b19808
https://hg.mozilla.org/mozilla-central/rev/33c8e33b0bb0
https://hg.mozilla.org/mozilla-central/rev/754c160ecbed
https://hg.mozilla.org/mozilla-central/rev/7c84e5453411
https://hg.mozilla.org/mozilla-central/rev/ab4d4dfee074
https://hg.mozilla.org/mozilla-central/rev/ca6e6783a379
https://hg.mozilla.org/mozilla-central/rev/87882fa5ef49
https://hg.mozilla.org/mozilla-central/rev/f26d5e27e1b6