Closed
Bug 1309163
Opened 6 years ago
Closed 6 years ago
Add H264 PPS decoder
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: alwu, Assigned: jya)
References
Details
Attachments
(4 files)
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.
Assignee | ||
Comment 1•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: alwu → jyavenard
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Attachment #8803849 -
Flags: review?(gsquelart)
Comment 7•6 years ago
|
||
mozreview-review |
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 8•6 years ago
|
||
mozreview-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 9•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 11•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review-reply |
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 13•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
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 15•6 years ago
|
||
mozreview-review |
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 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8803851 [details] Bug 1309163: P3. Add PPS decoder. https://reviewboard.mozilla.org/r/88056/#review87228
Attachment #8803851 -
Flags: review?(giles) → review+
Assignee | ||
Comment 17•6 years ago
|
||
mozreview-review-reply |
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
Reporter | ||
Comment 18•6 years ago
|
||
(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)
Assignee | ||
Comment 19•6 years ago
|
||
(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)
Updated•6 years ago
|
Priority: -- → P3
Reporter | ||
Comment 20•6 years ago
|
||
Hi, Jya, Could we land this bug now? Thanks!
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 21•6 years ago
|
||
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)
Reporter | ||
Comment 22•6 years ago
|
||
(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.
Assignee | ||
Comment 23•6 years ago
|
||
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;
Reporter | ||
Comment 24•6 years ago
|
||
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?
Reporter | ||
Comment 25•6 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(jyavenard)
Reporter | ||
Comment 26•6 years ago
|
||
Could we land this bug first, and then land the bug1321164?
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 27•6 years ago
|
||
I'm currently rebasing the patches and addressing the various comments. Should be done shortly.
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•6 years ago
|
||
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
Comment 35•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c58bfbe28006 https://hg.mozilla.org/mozilla-central/rev/99d76c84603d https://hg.mozilla.org/mozilla-central/rev/8930cf031f64 https://hg.mozilla.org/mozilla-central/rev/566e418afd5e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•