1111328 - Extract video dimensions from SPS NAL and do not rely on demuxer's data

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

In bug 1105771; we found that it is possible for a MSE stream to use a common init segment across all video streams.

This causes the video dimension parameters provided to the Apple's H264 decoder to not always match the actual video being decoded.

Apple's VideoToolbox requires to define the output frame size before the decoder is created. Using the wrong dimensions at the creation of the decoder results in decoded frame to be of the incorrect size.

Rather than rely on the dimensions provided in the init segment, we should extract those dimensions from the SPS NAL instead which are guaranteed to be correct.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Add H264 SPS NAL decoder

Add SPS decoder.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Retrieve video dimensions from H264 SPS when possible

Retrieve video dimensions from H264 SPS when possible

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8540435 [details] [diff] [review]
Add H264 SPS NAL decoder

Review of attachment 8540435 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/H264.cpp
@@ +20,5 @@
> +  explicit BitReader(const ByteBuffer& aBuffer)
> +  : m_br(aBuffer.Elements(), aBuffer.Length())
> +  {
> +  }
> +

suggestion: Warning in the case of leftover bits may help finding errors. Could use NS_ASSERTION()

@@ +31,5 @@
> +    return m_br.getBits(1);
> +  }
> +
> +  uint32_t ReadBits(size_t aNum)
> +  {

MOZ_ASSERT(aNum <= 32);

@@ +44,5 @@
> +  uint32_t ReadUE()
> +  {
> +    uint32_t i = 0;
> +
> +    while ((ReadBit() == 0) && (i < 32)) {

nit: Extra brackets are unnecessary

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 8540435 [details] [diff] [review]
Add H264 SPS NAL decoder

Review of attachment 8540435 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, that's a lot of code to read width/height!

::: media/libstagefright/binding/H264.cpp
@@ +48,5 @@
> +    while ((ReadBit() == 0) && (i < 32)) {
> +      i++;
> +    }
> +    uint32_t r = ReadBits(i);
> +    r += (1 << i) - 1;

Can't this end up with (1 << 32) which is undefined?

I also don't understand what's Golomb about this power-of-two thing...

@@ +64,5 @@
> +    }
> +  }
> +
> +private:
> +  stagefright::ABitReader m_br;

If you're using MOZ_ASSERT, you might as well use mozilla-style mBitReader member names.

::: media/libstagefright/binding/include/mp4_demuxer/H264.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef _MP4_DEMUXER_H264_H_
> +#define _MP4_DEMUXER_H264_H_

Local style seems to be only trailing underscore.

Mozilla style is no initial or trailing underscores and caps matching the filename, i.e. mp4_demuxer_H264_h.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

@@ +13,5 @@
> +class BitReader;
> +
> +struct SPSData
> +{
> +  /* Decoded Members */

These members are so nicely commented, I hardly know what to say!

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #4)
> Comment on attachment 8540435 [details] [diff] [review]
> Add H264 SPS NAL decoder
> 
> Review of attachment 8540435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow, that's a lot of code to read width/height!

LOL, it wouldn't be fun it that was easy.. They also have to justify the fees they want to charge for using that codec.

> 
> ::: media/libstagefright/binding/H264.cpp
> @@ +48,5 @@
> > +    while ((ReadBit() == 0) && (i < 32)) {
> > +      i++;
> > +    }
> > +    uint32_t r = ReadBits(i);
> > +    r += (1 << i) - 1;
> 
> Can't this end up with (1 << 32) which is undefined?

that is true; however it would mean the encoding was wrong to start with and as such we can't expect a valid result regardless.

> 
> I also don't understand what's Golomb about this power-of-two thing...

ultimately, it saves space... 

I used the implementation of the ITU H264 description as base:

leadingZeroBits = −1
for( b = 0; !b; leadingZeroBits++ )
b = read_bits( 1 )
The variable codeNum is then assigned as follows:
codeNum = 2^leadingZeroBits − 1 + read_bits( leadingZeroBits )

I thought about optimising it further, but gave up.


> > +#ifndef _MP4_DEMUXER_H264_H_
> > +#define _MP4_DEMUXER_H264_H_
> 
> Local style seems to be only trailing underscore.

I'm starting to worry that you could notice such stuff :)

> > +struct SPSData
> > +{
> > +  /* Decoded Members */
> 
> These members are so nicely commented, I hardly know what to say!

:) 80% of the SPS decoder time was spent on find the appropriate and copy/pasting the ITU documentation !

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> Comment on attachment 8540435 [details] [diff] [review]
> Add H264 SPS NAL decoder
> 
> Review of attachment 8540435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/binding/H264.cpp
> @@ +20,5 @@
> > +  explicit BitReader(const ByteBuffer& aBuffer)
> > +  : m_br(aBuffer.Elements(), aBuffer.Length())
> > +  {
> > +  }
> > +
> 
> suggestion: Warning in the case of leftover bits may help finding errors.
> Could use NS_ASSERTION()

I do not read/parse the entire SPS NAL; I left out the whole VUI's nal_hrd. Only decoding the VUI to extract the aspect ratio should we need it in the future and the detection of the frame rate.

So causing an error if we have bit left over would always be triggered requiring to add like ByteReader a DiscardRemaining method to be always called.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Add H264 SPS NAL decoder

Carrying r+

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Retrieve video dimensions from H264 SPS when possible

Add safety check on dimensions just in case

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Add H264 SPS NAL decoder part #2 (VUI and HDR)

While investigating which value could more accurately describe the amount of frames to keep in a queue for reordering, I implemented full VUI and HRD decoding. Unused in the end but just in case.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8540590 [details] [diff] [review]
Add H264 SPS NAL decoder part #2 (VUI and HDR)

Review of attachment 8540590 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/H264.cpp
@@ +275,5 @@
> +  if (aBr.ReadBit()) { // nal_hrd_parameters_present_flag
> +    hrd_parameters(aBr);
> +    hrd_present = true;
> +  }
> +  if (aBr.ReadBit()) { // vlc_hrd_parameters_present_flag

vcl_hrd_parameters_present_flag?

::: media/libstagefright/binding/include/mp4_demuxer/H264.h
@@ +107,5 @@
>      delta_pic_order_always_zero_flag equal to 1 specifies that
>      delta_pic_order_cnt[ 0 ] and delta_pic_order_cnt[ 1 ] are
>      not present in the slice headers of the sequence and shall
>      be inferred to be equal to
> +    0.

Move the zero to the line above.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8540578 [details] [diff] [review]
Add H264 SPS NAL decoder

Review of attachment 8540578 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/H264.cpp
@@ +38,5 @@
> +    if (mBitReader.numBitsLeft() < aNum) {
> +      return 0;
> +    }
> +    return mBitReader.getBits(aNum);
> +  }

BTW, you can same some duplication here by making ReadBit() just call ReadBits(1).

ABitReader.getBits(0) returns 0. Is this desired behaviour for your BitReader too, or should you assert for aNum == 0?

@@ +49,5 @@
> +    while (ReadBit() == 0 && i < 32) {
> +      i++;
> +    }
> +    uint32_t r = ReadBits(i);
> +    r += (1 << i) - 1;

if i=32 would be an invalid value, please reject it here. It's better to have a failure than proceed with undefined values.

Should assert and return 0 if i >= 32 anyway, because it's possible to get there without having seen a zero, which means the data's corrupt or we've lost sync.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8540590 [details] [diff] [review]
Add H264 SPS NAL decoder part #2 (VUI and HDR)

Review of attachment 8540590 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/H264.cpp
@@ +300,5 @@
> +H264::hrd_parameters(BitReader& aBr)
> +{
> +  uint32_t cpb_cnt_minus1 = aBr.ReadUE();
> +  aBr.ReadBits(4); // bit_rate_scale
> +  aBr.ReadBits(4); //cpb_size_scale

nit: space after //

Comment 13

[Ralph Giles (:rillian)]

(In reply to Jean-Yves Avenard [:jya] from comment #5)

> > I also don't understand what's Golomb about this power-of-two thing...
> 
> ultimately, it saves space... 

Yeah, I found the spec bit. Turns out I was thinking of Golumb rulers, which is a different thing, with no powers of two.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8540584 [details] [diff] [review]
Retrieve video dimensions from H264 SPS when possible

Review of attachment 8540584 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: dom/media/fmp4/apple/AppleVDADecoder.cpp
@@ +53,5 @@
> +      spsdata.pic_width && spsdata.pic_height) {
> +    mPictureWidth = spsdata.pic_width;
> +    mPictureHeight = spsdata.pic_height;
> +  }
> +

Do you want to validate these values here and reject too-large video? I suppose ValidatedFrame in MediaData.cpp will catch it eventually, but better to error early?

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #11)

> BTW, you can same some duplication here by making ReadBit() just call
> ReadBits(1).

good point.

> 
> ABitReader.getBits(0) returns 0. Is this desired behaviour for your
> BitReader too, or should you assert for aNum == 0?

it can't be called with 0 ; and I can't see anything wrong with returning 0, actually, this is what you want as in all H264 specs, having a value undefined should always make it considered be 0.

> 
> @@ +49,5 @@
> > +    while (ReadBit() == 0 && i < 32) {
> > +      i++;
> > +    }
> > +    uint32_t r = ReadBits(i);
> > +    r += (1 << i) - 1;
> 
> if i=32 would be an invalid value, please reject it here. It's better to
> have a failure than proceed with undefined values.

ok

> 
> Should assert and return 0 if i >= 32 anyway, because it's possible to get
> there without having seen a zero, which means the data's corrupt or we've
> lost sync.

ok

Comment 16

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #14)
> Do you want to validate these values here and reject too-large video? I
> suppose ValidatedFrame in MediaData.cpp will catch it eventually, but better
> to error early?

but what is a too large value ? :)

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Add H264 SPS NAL decoder

Carrying 2nd r+

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: Add H264 SPS NAL decoder part #2 (VUI and HDR)

Carrying r+

Comment 19

[Jean-Yves Avenard [:jya]]

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

Comment 20

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77aaf881effd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/becda7afe44f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/27328268f50a

Comment 21

[Ralph Giles (:rillian)]

(In reply to Jean-Yves Avenard [:jya] from comment #16)

> but what is a too large value ? :)

MAX_VIDEO_WIDTH and MAX_VIDEO_HEIGHT in VideoUtils.h are where we keep the allowed framesize limits; usually multiplied together so we don't reject portrait video. Compare with implementations in isValidVideoRegion() and ValidatePlane().

http://dxr.mozilla.org/mozilla-central/source/dom/media/VideoUtils.cpp#177
http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaData.cpp#51

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/77aaf881effd
https://hg.mozilla.org/mozilla-central/rev/becda7afe44f
https://hg.mozilla.org/mozilla-central/rev/27328268f50a

Comment 23

[Ralph Giles (:rillian)]

Comment on attachment 8540584 [details] [diff] [review]
Retrieve video dimensions from H264 SPS when possible

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing; sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: landed on m-c.
[Risks and why]: Low. This changes video decoder setup for more than just MSE, but is a straightforward implementation of the spec and the functional change is easy to revert.
[String/UUID change made/needed]: None.

This request applies to all patches in this bug.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8aa1f6c304b9
https://hg.mozilla.org/releases/mozilla-aurora/rev/16b5293c6dde
https://hg.mozilla.org/releases/mozilla-aurora/rev/74eed8eb1ce5

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c15e189d452

https://treeherder.mozilla.org/logviewer.html#?job_id=487600&repo=mozilla-aurora

Comment 26

[Ralph Giles (:rillian)]

Attachment: Add missing include.

Fix a problem with non-unified builds. This blocked the Aurora uplift.

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8544195 [details] [diff] [review]
Add missing include.

Review of attachment 8544195 [details] [diff] [review]:
-----------------------------------------------------------------

Weird it would compile on central...

Comment 28

[Ralph Giles (:rillian)]

> Weird it would compile on central...

Indeed. Maybe we got lucky and the non-unified builds haven't run yet?

https://hg.mozilla.org/integration/mozilla-inbound/rev/4356ad137ddf

Comment 29

[Ralph Giles (:rillian)]

Trying again with Aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f5ada037d91
https://hg.mozilla.org/releases/mozilla-aurora/rev/e736d73c7518
https://hg.mozilla.org/releases/mozilla-aurora/rev/74530165885b
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ded1424ac2f

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4356ad137ddf