Closed Bug 1314533 Opened 8 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1314533: [MSE] P1. Change member prototype.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314533: [MSE] P2. Reject appendBuffer if invalid data found.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314533: [MSE] P3. Reject invalid MP4 boxes.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314533: [MSE] P4. Reject invalid webm block.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314533: P5. Abort when webm parser encounter an error.
58 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1314533: [MSE] P6. Allow to detect error during preliminary container parsing.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314533: Do not expect init segment to be parsed when appending corrupted data.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314533: [MSE] P8. Update wpt expected results.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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.
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).
See Also: → 1315211
Assignee: nobody → jyavenard
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...'.
Attachment #8807989 - Flags: review?(gsquelart) → review+
Comment on attachment 8807990 [details]
Bug 1314533: [MSE] P2. Reject appendBuffer if invalid data found.

https://reviewboard.mozilla.org/r/90904/#review90608
Attachment #8807990 - Flags: review?(gsquelart) → review+
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!
That's disappointing of clang. :-(
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.
Attachment #8807991 - Flags: review?(gsquelart) → review+
Comment on attachment 8807992 [details]
Bug 1314533: [MSE] P4. Reject invalid webm block.

https://reviewboard.mozilla.org/r/90908/#review90614
Attachment #8807992 - Flags: review?(gsquelart) → review+
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 on attachment 8807993 [details]
Bug 1314533: P5. Abort when webm parser encounter an error.

https://reviewboard.mozilla.org/r/90910/#review90628
Attachment #8807993 - Flags: review?(kinetik) → review+
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?
Attachment #8807994 - Flags: review?(gsquelart) → review+
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 on attachment 8807996 [details]
Bug 1314533: [MSE] P8. Update wpt expected results.

https://reviewboard.mozilla.org/r/90916/#review90632
Attachment #8807996 - Flags: review?(gsquelart) → review+
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
Attachment #8807995 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
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
Depends on: 1322587
Depends on: 1329480
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: