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

RESOLVED FIXED in Firefox 36

Status

()

defect
P5
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla37
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 1114381
Assignee

Comment 1

5 years ago
Posted patch Add H264 SPS NAL decoder (obsolete) — Splinter Review
Add SPS decoder.
Attachment #8540435 - Flags: review?(ajones)
Assignee

Updated

5 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee

Comment 2

5 years ago
Retrieve video dimensions from H264 SPS when possible
Attachment #8540436 - Flags: review?(giles)
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
Attachment #8540435 - Flags: review?(ajones) → review+
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!
Attachment #8540435 - Flags: review+
Assignee

Comment 5

5 years ago
(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 !
Assignee

Comment 6

5 years ago
(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.
Assignee

Updated

5 years ago
Blocks: 1114861
Assignee

Comment 7

5 years ago
Posted patch Add H264 SPS NAL decoder (obsolete) — Splinter Review
Carrying r+
Assignee

Updated

5 years ago
Attachment #8540435 - Attachment is obsolete: true
Assignee

Comment 8

5 years ago
Add safety check on dimensions just in case
Assignee

Updated

5 years ago
Attachment #8540436 - Attachment is obsolete: true
Attachment #8540436 - Flags: review?(giles)
Assignee

Updated

5 years ago
Attachment #8540584 - Flags: review?(giles)
Assignee

Comment 9

5 years ago
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.
Attachment #8540590 - Flags: review?(ajones)
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 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 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 //
Attachment #8540590 - Flags: review?(ajones) → review+
(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 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?
Attachment #8540584 - Flags: review?(giles) → review+
Assignee

Updated

5 years ago
Summary: Extra video dimension from SPS NAL and do not rely on demuxer's data → Extract video dimensions from SPS NAL and do not rely on demuxer's data
Assignee

Comment 15

5 years ago
(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
Assignee

Comment 16

5 years ago
(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 ? :)
Assignee

Comment 17

5 years ago
Carrying 2nd r+
Assignee

Updated

5 years ago
Attachment #8540578 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8540590 - Attachment is obsolete: true
(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
https://hg.mozilla.org/mozilla-central/rev/77aaf881effd
https://hg.mozilla.org/mozilla-central/rev/becda7afe44f
https://hg.mozilla.org/mozilla-central/rev/27328268f50a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8540584 - Flags: approval-mozilla-aurora?
Attachment #8540584 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Fix a problem with non-unified builds. This blocked the Aurora uplift.
Attachment #8544195 - Flags: review?(jyavenard)
Assignee

Comment 27

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

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

Weird it would compile on central...
Attachment #8544195 - Flags: review?(jyavenard) → review+
> 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
You need to log in before you can comment on or make changes to this bug.