Closed Bug 1309163 Opened 5 years ago Closed 5 years ago

Add H264 PPS decoder

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: alwu, Assigned: jya)

References

Details

Attachments

(4 files)

Bug 1309163: Add BitReader::BitsLeft method.
58 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1309163: P2. Add AnnexB::HasPPS methods.
58 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1309163: P3. Add PPS decoder.
58 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1309163: P4. Add missing members to SPSData.
58 bytes, text/x-review-board-request
rillian
: review+
Details 
For decoding h264 with DXVA API directly, we need PPS information to fill into the DXVA's buffer so that the decoding process can work correctly.
I was wondering..

Why bother with H264 DXVA API ? what will that add over MFT?

I can understand for VP9 as for some graphic adapter it's the only way you can get VP9 hardware decoding...
Assignee: alwu → jyavenard
Comment on attachment 8803850 [details]
Bug 1309163: P2. Add AnnexB::HasPPS methods.

https://reviewboard.mozilla.org/r/88054/#review86986

r+ with comments addressed:

::: media/libstagefright/binding/AnnexB.cpp:357
(Diff revision 1)
> +    uint16_t length = reader.ReadU16();
> +    if ((reader.PeekU8() & 0x1f) != 7) {

You should check if you can actually read/peek, otherwise you'll hit a MOZ_ASSERT(false) in debug, or just get zeroes (which should work as expected here, but it feels wrong.)

::: media/libstagefright/binding/AnnexB.cpp:359
(Diff revision 1)
> +  uint8_t numSps = reader.ReadU8() & 0x1f;
> +  // Skip over the included SPS.
> +  for (uint8_t i = 0; i < numSps; i++) {
> +    uint16_t length = reader.ReadU16();
> +    if ((reader.PeekU8() & 0x1f) != 7) {
> +      // Not a SPS NAL type.

'a' -> 'an'

::: media/libstagefright/binding/AnnexB.cpp:366
(Diff revision 1)
> +    }
> +    if (!reader.Read(length)) {
> +      return false;
> +    }
> +  }
> +  uint8_t numPps = reader.ReadU8();

Check if you can read before reading.
Attachment #8803850 - Flags: review?(gsquelart) → review+
Attachment #8803849 - Flags: review?(gsquelart)
Comment on attachment 8803849 [details]
Bug 1309163: Add BitReader::BitsLeft method.

https://reviewboard.mozilla.org/r/88052/#review86988
Attachment #8803849 - Flags: review?(gsquelart) → review+
Comment on attachment 8803852 [details]
Bug 1309163: P4. Add missing members to SPSData.

https://reviewboard.mozilla.org/r/88058/#review87154

::: media/libstagefright/binding/include/mp4_demuxer/H264.h:411
(Diff revision 1)
>      1), which are related to picture order counts for the bottom field of a
>      coded frame, are present in the slice headers for coded frames as specified
>      in clause 7.3.3. bottom_field_pic_order_in_frame_present_flag equal to 0
>      specifies that the syntax elements delta_pic_order_cnt_bottom and
> -    delta_pic_order_cnt[ 1 ] are not present inthe slice headers. */
> +    delta_pic_order_cnt[ 1 ] are not present inthe slice headers.
> +    A.K.A pic_order_present_flag */

Space between 'inthe'.

A.K.A ? That's usually "Also known as". Might as well write that out; there's room. Also period at the end, please.
Comment on attachment 8803852 [details]
Bug 1309163: P4. Add missing members to SPSData.

https://reviewboard.mozilla.org/r/88058/#review87160

I don't see any code enforcing the derived or constrained values for the flags mentioned in the comments. Is that not an issue with SPS data?
Attachment #8803852 - Flags: review?(giles) → review+
Comment on attachment 8803852 [details]
Bug 1309163: P4. Add missing members to SPSData.

https://reviewboard.mozilla.org/r/88058/#review87160

which flags?

they are initialised to what the default requires. In all the cases where the spec states something like "if blah is false, then foo should be set to false", this is always the default value anyway.

There's not much point choking on those, the h264 decoder itself will handle those errors.
Comment on attachment 8803850 [details]
Bug 1309163: P2. Add AnnexB::HasPPS methods.

https://reviewboard.mozilla.org/r/88054/#review86986

> You should check if you can actually read/peek, otherwise you'll hit a MOZ_ASSERT(false) in debug, or just get zeroes (which should work as expected here, but it feels wrong.)

this is an issue in the existing code.. so I will take this in another patch.
Comment on attachment 8803850 [details]
Bug 1309163: P2. Add AnnexB::HasPPS methods.

https://reviewboard.mozilla.org/r/88054/#review86986

> Check if you can read before reading.

same as the first issue. will take that in another patch, adding some sanity checks.
Comment on attachment 8803852 [details]
Bug 1309163: P4. Add missing members to SPSData.

https://reviewboard.mozilla.org/r/88058/#review87160

So `bit_depth_luma_minus8` and `bit_depth_chroma_minus8` take their fallback `0` values through the PodZero call in the constructor. Ok.

That leaves `direct_8x8_inference_flag` which is supposed to be true if `frame_mbs_only_flag` is false. It would be nice to know if that's a constraint, or and override. If it's a constraint we should reject the file here, or if we can't because we're expected to play broken files, we should document that here.
Comment on attachment 8803852 [details]
Bug 1309163: P4. Add missing members to SPSData.

https://reviewboard.mozilla.org/r/88058/#review87160

direct_8x8_inference_flag is always read. Not knowing enough about the inner details of a h264 decoder, I guess we could simply display a warning, and let that slide for the actual decoder to handle.

will add a comment about it.
Comment on attachment 8803851 [details]
Bug 1309163: P3. Add PPS decoder.

https://reviewboard.mozilla.org/r/88056/#review87216

::: media/libstagefright/binding/H264.cpp:62
(Diff revision 1)
> +   30, 30, 32, 32, 32, 33, 33, 35
> +};
> +
> +static void
> +scaling_list(BitReader& aBr, uint8_t* aScalingList, int aSizeOfScalingList,
> +             const uint8_t* aDefaultList, const uint8_t* aFallbackList)

You should bounds-check your copy lengths here. You're always copying into arrays the compiler knows the size of. If references and ArrayLength() are too ugly, you could use vectors instead.

::: media/libstagefright/binding/H264.cpp:113
(Diff revision 1)
>  }
>  
> +const uint8_t H264::ZZ_SCAN[16] = { 0,  1,  4,  8,
> +                                    5,  2,  3,  6,
> +                                    9, 12, 13, 10,
> +                                    7, 11, 14, 15 };

I'm surprised this works. It doesn't look like a normal zig-zag read-out order, or match Figure 8-8/Table 8-13 in the spec. Is there some extra re-ordering happening somewhere? If so, it would be good to document that.

Same for H264::ZZ_SCAN8
Comment on attachment 8803851 [details]
Bug 1309163: P3. Add PPS decoder.

https://reviewboard.mozilla.org/r/88056/#review87228
Attachment #8803851 - Flags: review?(giles) → review+
Comment on attachment 8803851 [details]
Bug 1309163: P3. Add PPS decoder.

https://reviewboard.mozilla.org/r/88056/#review87216

> I'm surprised this works. It doesn't look like a normal zig-zag read-out order, or match Figure 8-8/Table 8-13 in the spec. Is there some extra re-ordering happening somewhere? If so, it would be good to document that.
> 
> Same for H264::ZZ_SCAN8

Its the same array ive found on all decoders out there: FFMPEG, mesa, OMX,  reference decoder or mplayer. 

They all access the 4X4 as a 16 ints vector, doing so:
for (i=0; i<10; i++) { newtab[i] = tab[zzscan[i]]; }

I copied the tables from ffmpeg
(In reply to Jean-Yves Avenard [:jya] from comment #1)
> I was wondering..
> 
> Why bother with H264 DXVA API ? what will that add over MFT?
> 
> I can understand for VP9 as for some graphic adapter it's the only way you
> can get VP9 hardware decoding...

What I discussed with cpearce was that we want to have fully control about DXVA so that we can reduce our crashing rate when using the DXVA.

Or you think we should implement the DXVA API for VPX first, instead of h264?
Flags: needinfo?(jyavenard)
(In reply to Alastor Wu [:alwu] from comment #18)
> What I discussed with cpearce was that we want to have fully control about
> DXVA so that we can reduce our crashing rate when using the DXVA.
> 
> Or you think we should implement the DXVA API for VPX first, instead of h264?

yes, we should do VP9 DXVA first, and consider h264 later.
VP9 DXVA is more important than h264 at this stage as we don't have a vp9 HW acceleration path at this stage except for intel chipsets.

Only DXVA is officially supported for VP9
Flags: needinfo?(jyavenard)
Hi, Jya,
Could we land this bug now?
Thanks!
Flags: needinfo?(jyavenard)
we could..

but I don't think it's usable as is because we don't handle more than one PPS / SPS configuration, we need a new data structure that would contain an array of SPS and PPS and cross check references.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #21)
> we could..
> 
> but I don't think it's usable as is because we don't handle more than one
> PPS / SPS configuration, we need a new data structure that would contain an
> array of SPS and PPS and cross check references.

Why we need to handle multiple PPS/SPS configuration? 
For my understanding, everytime the configuration changes, we would re-recreate new decoder, for the decoder, it has already known what's the current configuration.
SPS yes, however you can have multiple PPS within a single stream. I'm not sure how common are those, but FFmpeg handles this case.
The H264Context has a H264ParamSets field.

typedef struct H264ParamSets {
    AVBufferRef *sps_list[MAX_SPS_COUNT];
    AVBufferRef *pps_list[MAX_PPS_COUNT];

    AVBufferRef *pps_ref;
    AVBufferRef *sps_ref;
    /* currently active parameters sets */
    const PPS *pps;
    const SPS *sps;
} H264ParamSets;
Do you mean we need to implement a new data structure in the VideoInfo to store the multiple SPS/PPS? Or can we notify decoder everytime when the PPS changed?

--

And what's the meaning when we have multiple SPS/PPS? I mean, if one MediaRawData have two different PPS, how do I know which PPS I should use for decoding?

---

In present codebase, the input raw data would be sent into H264Converter::Input and check whether the SPS changed. In CheckForSPSChange(), we just compare two extra data, we didn't check the internal content. Is any possible that two extra data have same SPS, but they're not equal because they have different PPS?
Hi, Jya,

About handling multiple SPS/PPS, why we don't need to do that in present codes? If we want to do that, do you plan to implement in this bug or file another bug? (I can take it)

---

Following is my understanding, please correct me if I'm wrong.

First, we create the similar class like h264, eg. ParameterSets, and have new method which allow me to get all information I need, eg. DecodeParameterSetFromExtraData().

In [1], we can have the number of SPS NALUs (also similar for PPS), so we can have a for-loop to decode multiple SPS and store them into ParameterSets.

[1] https://goo.gl/WD9AE6

---

Decode PPS also need to have SPS, but if we have multiple SPS, which SPS would need to be used for decoding correspond PPS? 

---

In [2], we compare the extra data to decide whether the SPS changed. However, if the new extra data's SPS doesn't be changed, but its PPS data changed, we would also recreate the decoder. Is it reasonable?

[2] https://goo.gl/Vd9RnE

---

Thanks!
Flags: needinfo?(jyavenard)
Blocks: 1321164
Flags: needinfo?(jyavenard)
Blocks: 1319734
Could we land this bug first, and then land the bug1321164?
Flags: needinfo?(jyavenard)
I'm currently rebasing the patches and addressing the various comments. Should be done shortly.
Flags: needinfo?(jyavenard)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c58bfbe28006
Add BitReader::BitsLeft method. r=gerald
https://hg.mozilla.org/integration/autoland/rev/99d76c84603d
P2. Add AnnexB::HasPPS methods. r=gerald
https://hg.mozilla.org/integration/autoland/rev/8930cf031f64
P3. Add PPS decoder. r=rillian
https://hg.mozilla.org/integration/autoland/rev/566e418afd5e
P4. Add missing members to SPSData. r=rillian
Blocks: 1322571
Depends on: 1323081
You need to log in before you can comment on or make changes to this bug.