1412183 - Use BufferReader in mp4 parser

Status: RESOLVED FIXED

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

Description

[Alfredo Yang (:alfredo)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1387798#c2

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Bug 1412183 - use BufferReader instead of ByteReader in mp4 index parser.

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

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: Bug 1412183 - use BufferReader instead of ByteReader in AnnexB parser.

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

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: Bug 1412183 - use BufferReader instead of ByteReader in H264 parser.

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

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: Bug 1412183 - use BufferReader instead of ByteReader in DecoderData.

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

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8922621 [details]
Bug 1412183 - use BufferReader instead of ByteReader in mp4 index parser.

https://reviewboard.mozilla.org/r/193734/#review198858

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8922624 [details]
Bug 1412183 - use BufferReader instead of ByteReader in DecoderData.

https://reviewboard.mozilla.org/r/193740/#review198860

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 8922623 [details]
Bug 1412183 - use BufferReader instead of ByteReader in H264 parser.

https://reviewboard.mozilla.org/r/193738/#review198862

::: media/libstagefright/binding/H264.cpp:816
(Diff revision 1)
> -  ByteReader reader(aSample->Data(), aSample->Size());
> +  BufferReader reader(aSample->Data(), aSample->Size());
>  
>    while (reader.Remaining() >= nalLenSize) {
> -    uint32_t nalLen;
> +    uint32_t nalLen = 0;
>      switch (nalLenSize) {
> -      case 1: nalLen = reader.ReadU8();  break;
> +      case 1: reader.ReadU8().map([&] (uint8_t x) mutable { nalLen = x; return Ok(); }); break;

This is pretty awkward... another case where mozilla::Result needs unwrap_or to clean this up.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8922622 [details]
Bug 1412183 - use BufferReader instead of ByteReader in AnnexB parser.

https://reviewboard.mozilla.org/r/193736/#review198866

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Matthew Gregan [:kinetik] from comment #7)
> Comment on attachment 8922623 [details]
> Bug 1412183 - use BufferReader instead of ByteReader in H264 parser.
> 
> https://reviewboard.mozilla.org/r/193738/#review198862
> 
> ::: media/libstagefright/binding/H264.cpp:816
> (Diff revision 1)
> > -  ByteReader reader(aSample->Data(), aSample->Size());
> > +  BufferReader reader(aSample->Data(), aSample->Size());
> >  
> >    while (reader.Remaining() >= nalLenSize) {
> > -    uint32_t nalLen;
> > +    uint32_t nalLen = 0;
> >      switch (nalLenSize) {
> > -      case 1: nalLen = reader.ReadU8();  break;
> > +      case 1: reader.ReadU8().map([&] (uint8_t x) mutable { nalLen = x; return Ok(); }); break;
> 
> This is pretty awkward... another case where mozilla::Result needs unwrap_or
> to clean this up.

Happy to review patches to add things like this.

Comment 10

[Alfredo Yang (:alfredo)]

Comment on attachment 8922623 [details]
Bug 1412183 - use BufferReader instead of ByteReader in H264 parser.

https://reviewboard.mozilla.org/r/193738/#review198862

> This is pretty awkward... another case where mozilla::Result needs unwrap_or to clean this up.

Frankly speaking, I've no idea how to add unwrap_or in mozilla::Result. I'll add a comment to author and try to find a better way to rewrite it.

Comment 11

[Alfredo Yang (:alfredo)]

Comment on attachment 8922623 [details]
Bug 1412183 - use BufferReader instead of ByteReader in H264 parser.

https://reviewboard.mozilla.org/r/193738/#review198862

> Frankly speaking, I've no idea how to add unwrap_or in mozilla::Result. I'll add a comment to author and try to find a better way to rewrite it.

I change it slightly but no obvious improvement unfortunately.

Comment 12

[Alfredo Yang (:alfredo)]

Comment on attachment 8922621 [details]
Bug 1412183 - use BufferReader instead of ByteReader in mp4 index parser.

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

Comment 13

[Alfredo Yang (:alfredo)]

Comment on attachment 8922622 [details]
Bug 1412183 - use BufferReader instead of ByteReader in AnnexB parser.

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

Comment 14

[Alfredo Yang (:alfredo)]

Comment on attachment 8922623 [details]
Bug 1412183 - use BufferReader instead of ByteReader in H264 parser.

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

Comment 15

[Alfredo Yang (:alfredo)]

Comment on attachment 8922624 [details]
Bug 1412183 - use BufferReader instead of ByteReader in DecoderData.

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

Comment 16

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=631ae0fd37a7c0d3c4263db8af8069bc74be3b74

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de11750ca8ed
use BufferReader instead of ByteReader in mp4 index parser. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/f042748e5f83
use BufferReader instead of ByteReader in AnnexB parser. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/71bad9dae18b
use BufferReader instead of ByteReader in H264 parser. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/cce51f29025f
use BufferReader instead of ByteReader in DecoderData. r=kinetik

Comment 18

[Matthew Gregan [:kinetik]]

(In reply to Alfredo Yang (:alfredo) from comment #11)
> I change it slightly but no obvious improvement unfortunately.

I was imagining that:

    case 1: reader.ReadU8().map([&] (uint8_t x) mutable { nalLen = x; return Ok(); }); break;

becomes:

    case 1: nalLen = reader.ReadU8().unwrap_or(0); break;

...which is closer to the original code, and easier to read without the lamba.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/de11750ca8ed
https://hg.mozilla.org/mozilla-central/rev/f042748e5f83
https://hg.mozilla.org/mozilla-central/rev/71bad9dae18b
https://hg.mozilla.org/mozilla-central/rev/cce51f29025f

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8922621 [details]
Bug 1412183 - use BufferReader instead of ByteReader in mp4 index parser.

https://reviewboard.mozilla.org/r/193734/#review199920

::: media/libstagefright/binding/Index.cpp:182
(Diff revision 2)
>  
>        for (size_t i = 0; i < count; i++) {
> -        writer->mCrypto.mPlainSizes.AppendElement(reader.ReadU16());
> -        writer->mCrypto.mEncryptedSizes.AppendElement(reader.ReadU32());
> +        auto res_16 = reader.ReadU16();
> +        auto res_32 = reader.ReadU32();
> +        if (res_16.isErr() || res_32.isErr()) {
> +          return nullptr;

you can never enter there as we still have the test line 172

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8922622 [details]
Bug 1412183 - use BufferReader instead of ByteReader in AnnexB parser.

https://reviewboard.mozilla.org/r/193736/#review199926

::: media/libstagefright/binding/AnnexB.cpp:143
(Diff revision 2)
> -FindStartCodeInternal(ByteReader& aBr) {
> +FindStartCodeInternal(BufferReader& aBr) {
>    size_t offset = aBr.Offset();
>  
>    for (uint32_t i = 0; i < aBr.Align() && aBr.Remaining() >= 3; i++) {
> -    if (aBr.PeekU24() == 0x000001) {
> -      return true;
> +    auto res = aBr.PeekU24();
> +    if (res.isOk() && (res.unwrap() == 0x000001)) {

res will always be Ok, as the condition for the loop as aBr.Remaining() >= 3