| Message ID | 20200313102850.23913-14-anton@khirnov.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,01/18] mpeg4videodec: do not copy a range of fields at once | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | warning | Failed to apply patch |
On 3/13/2020 7:28 AM, Anton Khirnov wrote: > It represents the relationship between them more naturally and will be > useful in the following commits. > > Allows significantly more frames in fate-h264-attachment-631 to be > decoded. That sample is odd. It has an SPS/PPS pair in extradata that's wrong for the Buffering Period and Picture Timing SEI messages in-band, but seemingly correct to actually decode all these frames revealed by this patch. The SPS that makes the aforementioned SEI messages parseable, but the frames seemingly undecodeable, is in-band in the second RAP (Which fwiw reports a different bitstream level). So this patch prevents the extradata PPS from using the in-band SPS after it replaced the extradata one in sps_list. There's no PPS in-band. Is this really intended? Is the active PPS' sps_id meant to reference the SPS with that id that's currently "valid" and in sps_list, or the whichever one was valid at the time the PPS was first parsed?. If the latter, then why replace the SPS in-band but never the PPS? The in-band SPS is virtually unused after this change. > --- > libavcodec/h264_ps.c | 29 +++++- > libavcodec/h264_ps.h | 3 + > libavcodec/h264_slice.c | 14 +-- Missing changes to h264_parser.c, and one "h->ps.sps == (const SPS*)h->ps.sps_list[h->ps.pps->sps_id]->data" check in h264dec.c
Quoting James Almer (2020-03-18 17:05:38) > On 3/13/2020 7:28 AM, Anton Khirnov wrote: > > It represents the relationship between them more naturally and will be > > useful in the following commits. > > > > Allows significantly more frames in fate-h264-attachment-631 to be > > decoded. > > That sample is odd. It has an SPS/PPS pair in extradata that's wrong for > the Buffering Period and Picture Timing SEI messages in-band, but > seemingly correct to actually decode all these frames revealed by this > patch. > The SPS that makes the aforementioned SEI messages parseable, but the > frames seemingly undecodeable, is in-band in the second RAP (Which fwiw > reports a different bitstream level). So this patch prevents the > extradata PPS from using the in-band SPS after it replaced the extradata > one in sps_list. There's no PPS in-band. > > Is this really intended? Is the active PPS' sps_id meant to reference > the SPS with that id that's currently "valid" and in sps_list, or the > whichever one was valid at the time the PPS was first parsed?. If the > latter, then why replace the SPS in-band but never the PPS? > The in-band SPS is virtually unused after this change. My guess would be that someone (or some/thing/, dun dun dun) did some botched/misunderstood processing on that sample, so trying to figure out what was intended is futile. According to the spec, SPS contents must not change within a sequence (they can change on sequence boundaries), so this sample is invalid. From a more practical viewpoint, since a PPS is parsed for a given SPS I'd say it is clearer, cleaner and safer to tie them together like I'm doing in this patch. > > > --- > > libavcodec/h264_ps.c | 29 +++++- > > libavcodec/h264_ps.h | 3 + > > libavcodec/h264_slice.c | 14 +-- > > Missing changes to h264_parser.c, and one "h->ps.sps == (const > SPS*)h->ps.sps_list[h->ps.pps->sps_id]->data" check in h264dec.c Good catch, fixed locally.
On 3/23/2020 2:36 PM, Anton Khirnov wrote: > Quoting James Almer (2020-03-18 17:05:38) >> On 3/13/2020 7:28 AM, Anton Khirnov wrote: >>> It represents the relationship between them more naturally and will be >>> useful in the following commits. >>> >>> Allows significantly more frames in fate-h264-attachment-631 to be >>> decoded. >> >> That sample is odd. It has an SPS/PPS pair in extradata that's wrong for >> the Buffering Period and Picture Timing SEI messages in-band, but >> seemingly correct to actually decode all these frames revealed by this >> patch. >> The SPS that makes the aforementioned SEI messages parseable, but the >> frames seemingly undecodeable, is in-band in the second RAP (Which fwiw >> reports a different bitstream level). So this patch prevents the >> extradata PPS from using the in-band SPS after it replaced the extradata >> one in sps_list. There's no PPS in-band. >> >> Is this really intended? Is the active PPS' sps_id meant to reference >> the SPS with that id that's currently "valid" and in sps_list, or the >> whichever one was valid at the time the PPS was first parsed?. If the >> latter, then why replace the SPS in-band but never the PPS? >> The in-band SPS is virtually unused after this change. > > My guess would be that someone (or some/thing/, dun dun dun) did some > botched/misunderstood processing on that sample, so trying to figure out > what was intended is futile. I just find it bizarre that the extradata SPS makes the frames decode and the SEI unparseable, but the in-band SPS makes the SEI parseable and the frames undecodeable. > > According to the spec, SPS contents must not change within a sequence > (they can change on sequence boundaries), so this sample is invalid. > From a more practical viewpoint, since a PPS is parsed for a given SPS > I'd say it is clearer, cleaner and safer to tie them together like I'm > doing in this patch. There's commented out code in remove_sps() that used to drop all PPS that depended on a given SPS once a new one with the same id was found. I'm going to guess it was done to keep this mess of a sample working. Since now you're making this behavior official, you could remove that dead code while at it. > >> >>> --- >>> libavcodec/h264_ps.c | 29 +++++- >>> libavcodec/h264_ps.h | 3 + >>> libavcodec/h264_slice.c | 14 +-- >> >> Missing changes to h264_parser.c, and one "h->ps.sps == (const >> SPS*)h->ps.sps_list[h->ps.pps->sps_id]->data" check in h264dec.c > > Good catch, fixed locally. LGTM then.
Anton Khirnov: > Quoting James Almer (2020-03-18 17:05:38) >> On 3/13/2020 7:28 AM, Anton Khirnov wrote: >>> It represents the relationship between them more naturally and will be >>> useful in the following commits. >>> >>> Allows significantly more frames in fate-h264-attachment-631 to be >>> decoded. >> >> That sample is odd. It has an SPS/PPS pair in extradata that's wrong for >> the Buffering Period and Picture Timing SEI messages in-band, but >> seemingly correct to actually decode all these frames revealed by this >> patch. >> The SPS that makes the aforementioned SEI messages parseable, but the >> frames seemingly undecodeable, is in-band in the second RAP (Which fwiw >> reports a different bitstream level). So this patch prevents the >> extradata PPS from using the in-band SPS after it replaced the extradata >> one in sps_list. There's no PPS in-band. >> >> Is this really intended? Is the active PPS' sps_id meant to reference >> the SPS with that id that's currently "valid" and in sps_list, or the >> whichever one was valid at the time the PPS was first parsed?. If the >> latter, then why replace the SPS in-band but never the PPS? >> The in-band SPS is virtually unused after this change. > > My guess would be that someone (or some/thing/, dun dun dun) did some > botched/misunderstood processing on that sample, so trying to figure out > what was intended is futile. > > According to the spec, SPS contents must not change within a sequence > (they can change on sequence boundaries), so this sample is invalid. While this sample is invalid, isn't it possible to have a sample as follows: First a SPS and a PPS referencing the SPS, then a coded video sequence for which these SPS and PPS are active, then another IDR access unit with a new SPS that overwrites the old one, but with no new PPS. This is valid if I am not mistaken. With your patch, the second SPS would never be used. - Andreas
Quoting Andreas Rheinhardt (2020-03-23 23:08:14) > Anton Khirnov: > > Quoting James Almer (2020-03-18 17:05:38) > >> On 3/13/2020 7:28 AM, Anton Khirnov wrote: > >>> It represents the relationship between them more naturally and will be > >>> useful in the following commits. > >>> > >>> Allows significantly more frames in fate-h264-attachment-631 to be > >>> decoded. > >> > >> That sample is odd. It has an SPS/PPS pair in extradata that's wrong for > >> the Buffering Period and Picture Timing SEI messages in-band, but > >> seemingly correct to actually decode all these frames revealed by this > >> patch. > >> The SPS that makes the aforementioned SEI messages parseable, but the > >> frames seemingly undecodeable, is in-band in the second RAP (Which fwiw > >> reports a different bitstream level). So this patch prevents the > >> extradata PPS from using the in-band SPS after it replaced the extradata > >> one in sps_list. There's no PPS in-band. > >> > >> Is this really intended? Is the active PPS' sps_id meant to reference > >> the SPS with that id that's currently "valid" and in sps_list, or the > >> whichever one was valid at the time the PPS was first parsed?. If the > >> latter, then why replace the SPS in-band but never the PPS? > >> The in-band SPS is virtually unused after this change. > > > > My guess would be that someone (or some/thing/, dun dun dun) did some > > botched/misunderstood processing on that sample, so trying to figure out > > what was intended is futile. > > > > According to the spec, SPS contents must not change within a sequence > > (they can change on sequence boundaries), so this sample is invalid. > > While this sample is invalid, isn't it possible to have a sample as > follows: First a SPS and a PPS referencing the SPS, then a coded video > sequence for which these SPS and PPS are active, then another IDR access > unit with a new SPS that overwrites the old one, but with no new PPS. > This is valid if I am not mistaken. With your patch, the second SPS > would never be used. Hmm, this sounds pretty crazy, but I don't see anything in the spec forbidding this. But then again, we currently parse the PPS using values from the SPS. If the SPS changed, we'd have to re-parse the PPS again using new values. Since we are not doing that, we do not support this use case anyway.
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c index 708594954c..67e24c35f7 100644 --- a/libavcodec/h264_ps.c +++ b/libavcodec/h264_ps.c @@ -737,6 +737,15 @@ static int more_rbsp_data_in_pps(const SPS *sps, void *logctx) return 1; } +static void pps_free(void *opaque, uint8_t *data) +{ + PPS *pps = (PPS*)data; + + av_buffer_unref(&pps->sps_buf); + + av_freep(&data); +} + int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avctx, H264ParamSets *ps, int bit_length) { @@ -753,10 +762,15 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct return AVERROR_INVALIDDATA; } - pps_buf = av_buffer_allocz(sizeof(*pps)); - if (!pps_buf) + pps = av_mallocz(sizeof(*pps)); + if (!pps) return AVERROR(ENOMEM); - pps = (PPS*)pps_buf->data; + pps_buf = av_buffer_create((uint8_t*)pps, sizeof(*pps), + pps_free, NULL, 0); + if (!pps_buf) { + av_freep(&pps); + return AVERROR(ENOMEM); + } pps->data_size = gb->buffer_end - gb->buffer; if (pps->data_size > sizeof(pps->data)) { @@ -774,7 +788,14 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct ret = AVERROR_INVALIDDATA; goto fail; } - sps = (const SPS*)ps->sps_list[pps->sps_id]->data; + pps->sps_buf = av_buffer_ref(ps->sps_list[pps->sps_id]); + if (!pps->sps_buf) { + ret = AVERROR(ENOMEM); + goto fail; + } + pps->sps = (const SPS*)pps->sps_buf->data; + sps = pps->sps; + if (sps->bit_depth_luma > 14) { av_log(avctx, AV_LOG_ERROR, "Invalid luma bit depth=%d\n", diff --git a/libavcodec/h264_ps.h b/libavcodec/h264_ps.h index 2cc1cd6254..7c12930db1 100644 --- a/libavcodec/h264_ps.h +++ b/libavcodec/h264_ps.h @@ -133,6 +133,9 @@ typedef struct PPS { uint32_t dequant8_buffer[6][QP_MAX_NUM + 1][64]; uint32_t(*dequant4_coeff[6])[16]; uint32_t(*dequant8_coeff[6])[64]; + + AVBufferRef *sps_buf; + const SPS *sps; } PPS; typedef struct H264ParamSets { diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index 120f282c0a..211bcab680 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -1014,10 +1014,10 @@ static int h264_init_ps(H264Context *h, const H264SliceContext *sl, int first_sl h->ps.pps = (const PPS*)h->ps.pps_ref->data; } - if (h->ps.sps != (const SPS*)h->ps.sps_list[h->ps.pps->sps_id]->data) { + if (h->ps.sps != h->ps.pps->sps) { av_buffer_unref(&h->ps.sps_ref); h->ps.sps = NULL; - h->ps.sps_ref = av_buffer_ref(h->ps.sps_list[h->ps.pps->sps_id]); + h->ps.sps_ref = av_buffer_ref(h->ps.pps->sps_buf); if (!h->ps.sps_ref) return AVERROR(ENOMEM); h->ps.sps = (const SPS*)h->ps.sps_ref->data; @@ -1779,13 +1779,7 @@ static int h264_slice_header_parse(const H264Context *h, H264SliceContext *sl, return AVERROR_INVALIDDATA; } pps = (const PPS*)h->ps.pps_list[sl->pps_id]->data; - - if (!h->ps.sps_list[pps->sps_id]) { - av_log(h->avctx, AV_LOG_ERROR, - "non-existing SPS %u referenced\n", pps->sps_id); - return AVERROR_INVALIDDATA; - } - sps = (const SPS*)h->ps.sps_list[pps->sps_id]->data; + sps = pps->sps; sl->frame_num = get_bits(&sl->gb, sps->log2_max_frame_num); if (!first_slice) { @@ -2171,7 +2165,7 @@ int ff_h264_queue_decode_slice(H264Context *h, const H2645NAL *nal) av_log(h->avctx, AV_LOG_ERROR, "PPS changed between slices\n"); return AVERROR_INVALIDDATA; } - if (h->ps.sps != (const SPS*)h->ps.sps_list[h->ps.pps->sps_id]->data) { + if (h->ps.sps != pps->sps) { av_log(h->avctx, AV_LOG_ERROR, "SPS changed in the middle of the frame\n"); return AVERROR_INVALIDDATA; diff --git a/tests/ref/fate/h264-attachment-631 b/tests/ref/fate/h264-attachment-631 index ebb5eb4fcd..5ac45e70a4 100644 --- a/tests/ref/fate/h264-attachment-631 +++ b/tests/ref/fate/h264-attachment-631 @@ -3,6 +3,154 @@ #codec_id 0: rawvideo #dimensions 0: 720x480 #sar 0: 8/9 +0, 6, 6, 1, 518400, 0xd2068698 +0, 10, 10, 1, 518400, 0x2ee4865f +0, 14, 14, 1, 518400, 0x2a01b188 +0, 18, 18, 1, 518400, 0xa4bc9572 +0, 22, 22, 1, 518400, 0x4e882f72 +0, 26, 26, 1, 518400, 0xf79cfc9c +0, 30, 30, 1, 518400, 0x93afec23 +0, 34, 34, 1, 518400, 0xadf210e6 +0, 38, 38, 1, 518400, 0xb0bdd1f1 +0, 42, 42, 1, 518400, 0x4bbb4a24 +0, 46, 46, 1, 518400, 0x49db06c8 +0, 50, 50, 1, 518400, 0xf9781bfb +0, 54, 54, 1, 518400, 0xd3a373bc +0, 58, 58, 1, 518400, 0xccfb31c5 +0, 62, 62, 1, 518400, 0x276423a7 +0, 66, 66, 1, 518400, 0xb3729230 +0, 70, 70, 1, 518400, 0xeaf4586d +0, 74, 74, 1, 518400, 0x9e629b29 +0, 78, 78, 1, 518400, 0x921d6e58 +0, 82, 82, 1, 518400, 0xc988f527 +0, 86, 86, 1, 518400, 0x4e1fed4b +0, 90, 90, 1, 518400, 0xe3819724 +0, 94, 94, 1, 518400, 0xc07602ba +0, 98, 98, 1, 518400, 0xc6b1e8d0 +0, 102, 102, 1, 518400, 0x12d94755 +0, 106, 106, 1, 518400, 0x257a5264 +0, 110, 110, 1, 518400, 0x4f985461 +0, 114, 114, 1, 518400, 0x77577244 +0, 118, 118, 1, 518400, 0x81a59edf +0, 122, 122, 1, 518400, 0x9f33c0fa +0, 126, 126, 1, 518400, 0xa89cbb3f +0, 130, 130, 1, 518400, 0x6b1bcc1c +0, 134, 134, 1, 518400, 0x520acb74 +0, 138, 138, 1, 518400, 0x006dda91 +0, 142, 142, 1, 518400, 0x7377f96f +0, 146, 146, 1, 518400, 0x0b713224 +0, 150, 150, 1, 518400, 0x98943e53 +0, 154, 154, 1, 518400, 0x59f967e2 +0, 158, 158, 1, 518400, 0x976a2461 +0, 162, 162, 1, 518400, 0xcb4d3872 +0, 166, 166, 1, 518400, 0x0a174f59 +0, 170, 170, 1, 518400, 0x7cbe6c4f +0, 174, 174, 1, 518400, 0x475cbce4 +0, 178, 178, 1, 518400, 0x2c281bb9 +0, 182, 182, 1, 518400, 0xadee7826 +0, 186, 186, 1, 518400, 0x936059a6 +0, 190, 190, 1, 518400, 0xba09ae20 +0, 194, 194, 1, 518400, 0x355e94d7 +0, 198, 198, 1, 518400, 0xafc3a0b3 +0, 202, 202, 1, 518400, 0x55bd78af +0, 206, 206, 1, 518400, 0x9678c886 +0, 210, 210, 1, 518400, 0x4d69a62a +0, 214, 214, 1, 518400, 0x406e617c +0, 218, 218, 1, 518400, 0x7031ebdb +0, 222, 222, 1, 518400, 0xf862127d +0, 226, 226, 1, 518400, 0x619b8a53 +0, 230, 230, 1, 518400, 0x0fde6b72 +0, 234, 234, 1, 518400, 0xd137deff +0, 238, 238, 1, 518400, 0x9ae6ac2e +0, 242, 242, 1, 518400, 0x6cefb571 +0, 246, 246, 1, 518400, 0x7694dda2 +0, 250, 250, 1, 518400, 0x2253f6a2 +0, 254, 254, 1, 518400, 0x770db468 +0, 258, 258, 1, 518400, 0xf4c815a5 +0, 262, 262, 1, 518400, 0x0a0f38b6 +0, 266, 266, 1, 518400, 0x17490907 +0, 270, 270, 1, 518400, 0xbc362ed6 +0, 274, 274, 1, 518400, 0x24de1b5c +0, 278, 278, 1, 518400, 0x55d20b2a +0, 282, 282, 1, 518400, 0xca1af9b1 +0, 286, 286, 1, 518400, 0x7e7b7473 +0, 290, 290, 1, 518400, 0xed30dd23 +0, 294, 294, 1, 518400, 0xb694c58f +0, 298, 298, 1, 518400, 0x8270deb7 +0, 302, 302, 1, 518400, 0x91b3b4f7 +0, 306, 306, 1, 518400, 0x37fcb63c +0, 310, 310, 1, 518400, 0x7ebcafca +0, 314, 314, 1, 518400, 0x8508b6da +0, 318, 318, 1, 518400, 0xe7e0b15e +0, 322, 322, 1, 518400, 0x9618fa0e +0, 326, 326, 1, 518400, 0xd4c3b20c +0, 330, 330, 1, 518400, 0x1aad03d1 +0, 334, 334, 1, 518400, 0xb5c18e20 +0, 338, 338, 1, 518400, 0x70144034 +0, 342, 342, 1, 518400, 0x937ee203 +0, 346, 346, 1, 518400, 0x680d72ad +0, 350, 350, 1, 518400, 0x8c9647b1 +0, 354, 354, 1, 518400, 0x65fce70a +0, 358, 358, 1, 518400, 0xa3d785dd +0, 362, 362, 1, 518400, 0xaf1a54c2 +0, 366, 366, 1, 518400, 0x301c6f4c +0, 370, 370, 1, 518400, 0x0255b5ac +0, 374, 374, 1, 518400, 0x967da8de +0, 378, 378, 1, 518400, 0x1f7e6c8c +0, 382, 382, 1, 518400, 0xb41badbf +0, 386, 386, 1, 518400, 0xca853613 +0, 390, 390, 1, 518400, 0x9f8696cb +0, 394, 394, 1, 518400, 0x55ec8427 +0, 398, 398, 1, 518400, 0x08779f91 +0, 402, 402, 1, 518400, 0x171fbc34 +0, 406, 406, 1, 518400, 0x5e9c6ddd +0, 410, 410, 1, 518400, 0xd9a55786 +0, 414, 414, 1, 518400, 0xdb509948 +0, 418, 418, 1, 518400, 0x2a326178 +0, 422, 422, 1, 518400, 0x4842c411 +0, 426, 426, 1, 518400, 0x35399db4 +0, 430, 430, 1, 518400, 0xa182b9aa +0, 434, 434, 1, 518400, 0xb6df772d +0, 438, 438, 1, 518400, 0xfe61b651 +0, 442, 442, 1, 518400, 0x031cb305 +0, 446, 446, 1, 518400, 0xde553506 +0, 450, 450, 1, 518400, 0x24ab8557 +0, 454, 454, 1, 518400, 0xadf5e251 +0, 458, 458, 1, 518400, 0xb3a3c6c5 +0, 462, 462, 1, 518400, 0x9cedc6ac +0, 466, 466, 1, 518400, 0x6ddf9b26 +0, 470, 470, 1, 518400, 0x3bfaf200 +0, 474, 474, 1, 518400, 0x0337d6f1 +0, 478, 478, 1, 518400, 0x71367bc7 +0, 482, 482, 1, 518400, 0x9e1876b8 +0, 486, 486, 1, 518400, 0x37b89366 +0, 490, 490, 1, 518400, 0x6e349056 +0, 494, 494, 1, 518400, 0x718a9543 +0, 498, 498, 1, 518400, 0x48e46e57 +0, 502, 502, 1, 518400, 0xb2ae494c +0, 506, 506, 1, 518400, 0x0ec937dc +0, 510, 510, 1, 518400, 0xb1e88149 +0, 514, 514, 1, 518400, 0xedbba51d +0, 518, 518, 1, 518400, 0x8955d114 +0, 522, 522, 1, 518400, 0x951e8716 +0, 526, 526, 1, 518400, 0x119064de +0, 530, 530, 1, 518400, 0xc06bd99a +0, 534, 534, 1, 518400, 0xdfccd738 +0, 538, 538, 1, 518400, 0x6c2de0a5 +0, 542, 542, 1, 518400, 0x11c1fdf7 +0, 546, 546, 1, 518400, 0xdcd26a62 +0, 550, 550, 1, 518400, 0x0ff63f3d +0, 554, 554, 1, 518400, 0x6443382a +0, 558, 558, 1, 518400, 0x28ce5ce3 +0, 562, 562, 1, 518400, 0xe0d47fbd +0, 566, 566, 1, 518400, 0xfdc0beed +0, 570, 570, 1, 518400, 0x9adeddc4 +0, 574, 574, 1, 518400, 0x8e5669fc +0, 578, 578, 1, 518400, 0xf0beb8ae +0, 582, 582, 1, 518400, 0xbdd68806 +0, 586, 586, 1, 518400, 0xe3c6ae23 +0, 590, 590, 1, 518400, 0xeba952c1 +0, 594, 594, 1, 518400, 0x734ff153 0, 598, 598, 1, 518400, 0xc3c0f1cf 0, 603, 603, 1, 518400, 0x21a5df80 0, 607, 607, 1, 518400, 0x5b8e115b