Comment on attachment 8922621 [details] Bug 1412183 - use BufferReader instead of ByteReader in mp4 index parser. https://reviewboard.mozilla.org/r/193734/#review198858

Comment on attachment 8922624 [details] Bug 1412183 - use BufferReader instead of ByteReader in DecoderData. https://reviewboard.mozilla.org/r/193740/#review198860

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 on attachment 8922622 [details] Bug 1412183 - use BufferReader instead of ByteReader in AnnexB parser. https://reviewboard.mozilla.org/r/193736/#review198866

(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 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 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.

Pushed by ayang@mozilla.com: 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

(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.

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 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 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