From 40e80b1ba55eeb6fdf83d2f79ba1ea7edb941a78 Mon Sep 17 00:00:00 2001 From: Lorenzo Miniero Date: Wed, 21 Dec 2022 16:08:03 +0100 Subject: [PATCH] Fix overwriting of 7-bit PictureID when doing VP8 simulcast (#3121) --- fuzzers/rtp_fuzzer.c | 3 ++- src/rtp.c | 3 ++- src/utils.c | 20 +++++++++++++++----- src/utils.h | 3 ++- 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/fuzzers/rtp_fuzzer.c b/fuzzers/rtp_fuzzer.c index a82a51d095..0fdb005a8a 100644 --- a/fuzzers/rtp_fuzzer.c +++ b/fuzzers/rtp_fuzzer.c @@ -92,12 +92,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { janus_h264_is_keyframe(payload, plen); /* VP8 targets */ + gboolean m = FALSE; uint16_t picid = 0; uint8_t tlzi = 0, tid = 0, ybit = 0, keyidx = 0; janus_vp8_simulcast_context vp8_context; memset(&vp8_context, 0, sizeof(janus_vp8_simulcast_context)); janus_vp8_is_keyframe(payload, plen); - janus_vp8_parse_descriptor(payload, plen, &picid, &tlzi, &tid, &ybit, &keyidx); + janus_vp8_parse_descriptor(payload, plen, &m, &picid, &tlzi, &tid, &ybit, &keyidx); janus_vp8_simulcast_descriptor_update(copy_payload, plen, &vp8_context, TRUE); /* VP9 targets */ diff --git a/src/rtp.c b/src/rtp.c index 34dbf5ff26..55b02dbb03 100644 --- a/src/rtp.c +++ b/src/rtp.c @@ -1210,12 +1210,13 @@ gboolean janus_rtp_simulcasting_context_process_rtp(janus_rtp_simulcasting_conte /* Temporal layers are only available for VP8, so don't do anything else for other codecs */ if(vcodec == JANUS_VIDEOCODEC_VP8) { /* Check if there's any temporal scalability to take into account */ + gboolean m = FALSE; uint16_t picid = 0; uint8_t tlzi = 0; uint8_t tid = 0; uint8_t ybit = 0; uint8_t keyidx = 0; - if(janus_vp8_parse_descriptor(payload, plen, &picid, &tlzi, &tid, &ybit, &keyidx) == 0) { + if(janus_vp8_parse_descriptor(payload, plen, &m, &picid, &tlzi, &tid, &ybit, &keyidx) == 0) { //~ JANUS_LOG(LOG_WARN, "%"SCNu16", %u, %u, %u, %u\n", picid, tlzi, tid, ybit, keyidx); if(context->templayer != context->templayer_target && tid == context->templayer_target) { /* FIXME We should be smarter in deciding when to switch */ diff --git a/src/utils.c b/src/utils.c index fdaef71c4e..a5f5738304 100644 --- a/src/utils.c +++ b/src/utils.c @@ -916,7 +916,7 @@ gboolean janus_h265_is_keyframe(const char *buffer, int len) { } int janus_vp8_parse_descriptor(char *buffer, int len, - uint16_t *picid, uint8_t *tl0picidx, uint8_t *tid, uint8_t *y, uint8_t *keyidx) { + gboolean *m, uint16_t *picid, uint8_t *tl0picidx, uint8_t *tid, uint8_t *y, uint8_t *keyidx) { if(!buffer || len < 6) return -1; if(picid) @@ -951,6 +951,8 @@ int janus_vp8_parse_descriptor(char *buffer, int len, partpicid = (wholepicid & 0x7FFF); buffer++; } + if(m) + *m = (mbit ? TRUE : FALSE); if(picid) *picid = partpicid; } @@ -976,7 +978,7 @@ int janus_vp8_parse_descriptor(char *buffer, int len, return 0; } -static int janus_vp8_replace_descriptor(char *buffer, int len, uint16_t picid, uint8_t tl0picidx) { +static int janus_vp8_replace_descriptor(char *buffer, int len, gboolean m, uint16_t picid, uint8_t tl0picidx) { if(!buffer || len < 6) return -1; uint8_t vp8pd = *buffer; @@ -994,7 +996,7 @@ static int janus_vp8_replace_descriptor(char *buffer, int len, uint16_t picid, u buffer++; vp8pd = *buffer; uint8_t mbit = (vp8pd & 0x80); - if(!mbit) { + if(!mbit || !m) { *buffer = picid; } else { uint16_t wholepicid = htons(picid); @@ -1031,13 +1033,14 @@ void janus_vp8_simulcast_context_reset(janus_vp8_simulcast_context *context) { void janus_vp8_simulcast_descriptor_update(char *buffer, int len, janus_vp8_simulcast_context *context, gboolean switched) { if(!buffer || len < 0) return; + gboolean m = FALSE; uint16_t picid = 0; uint8_t tlzi = 0; uint8_t tid = 0; uint8_t ybit = 0; uint8_t keyidx = 0; /* Parse the identifiers in the VP8 payload descriptor */ - if(janus_vp8_parse_descriptor(buffer, len, &picid, &tlzi, &tid, &ybit, &keyidx) < 0) + if(janus_vp8_parse_descriptor(buffer, len, &m, &picid, &tlzi, &tid, &ybit, &keyidx) < 0) return; if(switched) { context->base_picid_prev = context->last_picid; @@ -1046,9 +1049,16 @@ void janus_vp8_simulcast_descriptor_update(char *buffer, int len, janus_vp8_simu context->base_tlzi = tlzi; } context->last_picid = (picid-context->base_picid)+context->base_picid_prev+1; + if(!m && context->last_picid > 127) { + context->last_picid -= 128; + if(context->last_picid > 127) + context->last_picid = 0; + } else if(m && context->last_picid > 32767) { + context->last_picid -= 32768; + } context->last_tlzi = (tlzi-context->base_tlzi)+context->base_tlzi_prev+1; /* Overwrite the values in the VP8 payload descriptors with the ones we have */ - janus_vp8_replace_descriptor(buffer, len, context->last_picid, context->last_tlzi); + janus_vp8_replace_descriptor(buffer, len, m, context->last_picid, context->last_tlzi); } /* Helper method to parse a VP9 RTP video frame and get some SVC-related info: diff --git a/src/utils.h b/src/utils.h index 4b0e1ec704..7f173b1172 100644 --- a/src/utils.h +++ b/src/utils.h @@ -356,6 +356,7 @@ void janus_vp8_simulcast_context_reset(janus_vp8_simulcast_context *context); /*! \brief Helper method to parse a VP8 payload descriptor for useful info (e.g., when simulcasting) * @param[in] buffer The RTP payload to process * @param[in] len The length of the RTP payload + * @param[out] m Whether the Picture ID is 15 bit or 7 bit * @param[out] picid The Picture ID * @param[out] tl0picidx Temporal level zero index * @param[out] tid Temporal-layer index @@ -363,7 +364,7 @@ void janus_vp8_simulcast_context_reset(janus_vp8_simulcast_context *context); * @param[out] keyidx Temporal key frame index * @returns 0 in case of success, a negative integer otherwise */ int janus_vp8_parse_descriptor(char *buffer, int len, - uint16_t *picid, uint8_t *tl0picidx, uint8_t *tid, uint8_t *y, uint8_t *keyidx); + gboolean *m, uint16_t *picid, uint8_t *tl0picidx, uint8_t *tid, uint8_t *y, uint8_t *keyidx); /*! \brief Use the context info to update the RTP header of a packet, if needed * @param[in] buffer The RTP payload to process