1323081 - Lots of WARNING: Invalid H.264 data

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Problem was introduced with bug 1321164 which enabled the PPS decoder.

It finds that all H264 content is invalid somehow.

Comment 1

[Alastor Wu [:alwu]]

Sorry I don't understand this bug well, what's this bug for? Checking the h264 content is valid or not?

Comment 2

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu] from comment #1)
> Sorry I don't understand this bug well, what's this bug for? Checking the
> h264 content is valid or not?

Ffmpeg trimmed trailing 0 of pps mal first. We don't. So we attempt to read past the end of the data which triggers the warnings.

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Alastor Wu [:alwu] from comment #1)
> Sorry I don't understand this bug well, what's this bug for? Checking the
> h264 content is valid or not?

Ffmpeg trimmed trailing 0 of pps mal first. We don't. So we attempt to read past the end of the data which triggers the warnings.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1323081: P1. Add native BitReader class.

This is a rewrite from Stagefright's ABitReader. The major difference is that you give the original size in bits rather than in bytes. ABitReader always read all bits available. While under some circumstances we want to bound the buffer to a set number of bits.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

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

Comment 6

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

Comment on attachment 8818465 [details]
Bug 1323081: P1. Add native BitReader class.

https://reviewboard.mozilla.org/r/98510/#review98780

r+, with behavior question:

::: media/libstagefright/binding/BitReader.cpp:42
(Diff revision 1)
> -  if (mBitReader->numBitsLeft() < aNum) {
> +  if (mTotalBitsLeft < aNum) {
> +    NS_ASSERTION(false, "Reading past end of buffer");
>      return 0;
>    }

So you want potential crashes in debug mode? (Just making sure that this new behavior is what you really intend.)

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818465 [details]
Bug 1323081: P1. Add native BitReader class.

https://reviewboard.mozilla.org/r/98510/#review98780

> So you want potential crashes in debug mode? (Just making sure that this new behavior is what you really intend.)

what crash?

This is a soft assertion
it will return 0, which means everything single read will fail after that and playback will fail.

We will unecessarily attempt to read many times past the data potentially, but seing that it's broken, well it's broken :)

Comment 8

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

Comment on attachment 8818465 [details]
Bug 1323081: P1. Add native BitReader class.

https://reviewboard.mozilla.org/r/98510/#review98780

> what crash?
> 
> This is a soft assertion
> it will return 0, which means everything single read will fail after that and playback will fail.
> 
> We will unecessarily attempt to read many times past the data potentially, but seing that it's broken, well it's broken :)

Oh I see, thanks.

Comment 9

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

Comment on attachment 8818466 [details]
Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

https://reviewboard.mozilla.org/r/98512/#review98786

r+ with micro-nit:

::: media/libstagefright/binding/H264.cpp:138
(Diff revision 1)
> +  uint8_t v = aNAL->ElementAt(size - 1);
> +
> +  if (size > UINT32_MAX / 8) {
> +    // We can't represent it, we'll use as much as we can.
> +    return UINT32_MAX;
> +  }

You don't need 'v' in the if-block, so I think you could swap the two.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818466 [details]
Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

https://reviewboard.mozilla.org/r/98512/#review98786

> You don't need 'v' in the if-block, so I think you could swap the two.

will do, and add a comment about what the code later does.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818465 [details]
Bug 1323081: P1. Add native BitReader class.

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

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818466 [details]
Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818465 [details]
Bug 1323081: P1. Add native BitReader class.

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

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818466 [details]
Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

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

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818466 [details]
Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

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

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818465 [details]
Bug 1323081: P1. Add native BitReader class.

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

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8818466 [details]
Bug 1323081: [H264] P2. Ignore NAL stop bit and trailing 0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98512/diff/4-5/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a0fa935cfc2
P1. Add native BitReader class. r=gerald
https://hg.mozilla.org/integration/autoland/rev/79c7322f34ec
[H264] P2. Ignore NAL stop bit and trailing 0. r=gerald

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a0fa935cfc2
https://hg.mozilla.org/mozilla-central/rev/79c7322f34ec