1321164 - Multiple SPS/PPS structure

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

Fork from bug1309163 comment23, we might have multiple SPS/PPS within a single stream, we need to have a new data structure to handle that.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1321164 - part1 : handle multiple SPS/PPS.

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

Comment 2

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

Hi, Jya,
Here is the preliminary draft, could you give me some feedback?
Thanks!

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

https://reviewboard.mozilla.org/r/96534/#review96736

LGTM. nothing much to add.
In the H264ParameterSet we probably want to mark what is the current SPSData index currently in use. And if that one change, then we have to tear down the decoder.
Which SPS is used is defined in the PPS.

Comment 4

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> LGTM. nothing much to add.
> In the H264ParameterSet we probably want to mark what is the current SPSData
> index currently in use. And if that one change, then we have to tear down
> the decoder.
> Which SPS is used is defined in the PPS.

If we want to know the current SPS, we need to know which one is current PPS, but there might also have multiple PPS. As my understanding, the way to know the current PPS is from slice header (spec 7.3.3), which would indicate the PPS index. (If so, that would be implemented in bug1319734)

Or we have another way to know which SPS/PPS is in use?

Comment 5

[Alastor Wu [:alwu]]

I'll increase the current SPS/PPS index in this bug and implement how to get the current SPS/PPS in bug1319734. 

FFmpeg sets the current SPS/PPS when parsing the NAL_SLICE [1].

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

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1321164 - part2 : use enum for NAL types.

In order to increase the readability, use enum value for the NAL types.

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

Comment 7

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

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

Comment 8

[James Cheng[:JamesCheng]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

https://reviewboard.mozilla.org/r/96836/#review97054

::: media/libstagefright/binding/include/mp4_demuxer/H264.h:35
(Diff revision 1)
> +    H264_NAL_SPS_EXT         = 13,
> +    H264_NAL_PREFIX          = 14,
> +    H264_NAL_AUXILIARY_SLICE = 19,
> +    H264_NAL_SLICE_EXT       = 20,
> +    H264_NAL_SLICE_EXT_DVC   = 21,
> +} NAL_TYPES;

Is it really compilable? It means define a variable but a declaration. I think you should write enum NAL_TYPES instead?

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

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

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

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

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

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

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

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

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

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

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

https://reviewboard.mozilla.org/r/96836/#review97076

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

https://reviewboard.mozilla.org/r/96534/#review97078

::: dom/media/fmp4/MP4Demuxer.cpp:255
(Diff revision 4)
>      mIsH264 = true;
>      RefPtr<MediaByteBuffer> extraData = videoInfo->mExtraData;
>      mNeedSPSForTelemetry = AccumulateSPSTelemetry(extraData);
> -    mp4_demuxer::SPSData spsdata;
> -    if (mp4_demuxer::H264::DecodeSPSFromExtraData(extraData, spsdata) &&
> -        spsdata.pic_width > 0 && spsdata.pic_height > 0 &&
> +    mp4_demuxer::SPSDataSet spsDataSet;
> +    if (mp4_demuxer::H264::DecodeSPSFromExtraData(extraData, spsDataSet)) {
> +      mp4_demuxer::SPSData& spsdata = spsDataSet[0];

const mp4_demuxer::SPSData&

::: media/libstagefright/binding/H264.cpp:680
(Diff revision 4)
>  
>    aDest.pic_parameter_set_id = br.ReadUE();
>    aDest.seq_parameter_set_id = br.ReadUE();
> +
> +  const SPSData* sps = &aSPSs[0];
> +  for (uint32_t idx = 0; idx < aSPSs.Length(); idx++) {

for (const auto& sps : aSPSs) {
 ...
}

::: media/libstagefright/binding/H264.cpp:685
(Diff revision 4)
> +  for (uint32_t idx = 0; idx < aSPSs.Length(); idx++) {
> +    if (aSPSs[idx].seq_parameter_set_id == aDest.seq_parameter_set_id) {
> +      sps = &aSPSs[idx];
> +      break;
> +    }
> +  }

shouldn't not finding it be an error?

ffmpeg certainly treats it as such.
They also check that the seq_parameter_set_id is less than MAX_SPS_COUNT.

The size of a SPS structure is rather small, instead of having to loop into an array, you could make the SPSSet an array of size MAX_SPS_COUNT. And store the SPS of the right id directly at the index.

Then to find the right SPS, it's just a matter of doing SPSDataSet[SPSid]
so you get constant access time of O(1)

::: media/libstagefright/binding/H264.cpp:789
(Diff revision 4)
>    }
>    return true;
>  }
>  
> +/* static */ bool
> +H264::DecodeParameterSetFromExtraData(const mozilla::MediaByteBuffer* aExtraData,

DecodeParametersSet seeing as you're decoding both the SPS and PPSes

::: media/libstagefright/binding/include/mp4_demuxer/H264.h:574
(Diff revision 4)
> +
> +struct H264ParameterSets
> +{
> +  SPSDataSet SPSs;
> +  PPSDataSet PPSs;
> +  PPSData* currentSPS;

make them const T* as they aren't supposed to be modified.

Comment 16

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #15)
> ::: media/libstagefright/binding/H264.cpp:685
> (Diff revision 4)
> > +  for (uint32_t idx = 0; idx < aSPSs.Length(); idx++) {
> > +    if (aSPSs[idx].seq_parameter_set_id == aDest.seq_parameter_set_id) {
> > +      sps = &aSPSs[idx];
> > +      break;
> > +    }
> > +  }
> 
> shouldn't not finding it be an error?
> 
> ffmpeg certainly treats it as such.
> They also check that the seq_parameter_set_id is less than MAX_SPS_COUNT.
> 
> The size of a SPS structure is rather small, instead of having to loop into
> an array, you could make the SPSSet an array of size MAX_SPS_COUNT. And
> store the SPS of the right id directly at the index.
> 
> Then to find the right SPS, it's just a matter of doing SPSDataSet[SPSid]
> so you get constant access time of O(1)

If we use the pattern "SPSDataSet[SPSid]", how do we get the current sps from DecodeSPSFromExtraData()?

We might have such kinds of situations
(1) one SPS
- return this SPS
(2) multiple SPSes, one PPS
- check the SPS_id in PPS, and return that one
(3) multiple SPSes, and multiple PPSes
- we need to check the PPS_id in slice so that we can know the SPS_id (but the slice doesn't exist in extradata)

If we got the case3, we can't know which SPS is in use from extradata, we also need to parse h264's byte buffer.

Or you think we could always use the first SPS as the current SPS?

Comment 17

[Jean-Yves Avenard [:jya]]

Could change the prototype of the function to return an int instead. A negative number is an error, otherwise it's the SPS id. 

The current situation isn't perfect anyway as it always assume the first sps found is the one we want. That's true 99.99% of the time only :)

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

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

Comment 19

[Alastor Wu [:alwu]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

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

Comment 20

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

Hi, Jya,
I made some changes, could you help me review this patch again?
Thanks!

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

this got r+ed on reviewboard

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8815678 [details]
Bug 1321164 - part1 : handle multiple SPS/PPS.

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

Comment 23

[Alastor Wu [:alwu]]

Comment on attachment 8816063 [details]
Bug 1321164 - part2 : use enum for NAL types.

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

Comment 24

[Alastor Wu [:alwu]]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9169c83424aa
part1 : handle multiple SPS/PPS. r=jya
https://hg.mozilla.org/integration/autoland/rev/06552bcdabec
part2 : use enum for NAL types. r=jya

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9169c83424aa
https://hg.mozilla.org/mozilla-central/rev/06552bcdabec