From d77fcf251b33691bbe52f21e061b79564000c9e8 Mon Sep 17 00:00:00 2001 From: Stian Selnes Date: Fri, 30 Oct 2020 03:09:48 +0100 Subject: [PATCH] rtpvp8depay: Send lost events when marker bit is missing This means the previous frame was incomplete. Part-of: --- gst/rtp/gstrtpvp8depay.c | 89 ++++++++++++++-- gst/rtp/gstrtpvp8depay.h | 1 + tests/check/elements/rtpvp8.c | 184 +++++++++++++++++++++++++++++++++- 3 files changed, 261 insertions(+), 13 deletions(-) diff --git a/gst/rtp/gstrtpvp8depay.c b/gst/rtp/gstrtpvp8depay.c index 6fe65a3fc0..40ce1e056b 100644 --- a/gst/rtp/gstrtpvp8depay.c +++ b/gst/rtp/gstrtpvp8depay.c @@ -80,6 +80,7 @@ gst_rtp_vp8_depay_init (GstRtpVP8Depay * self) self->adapter = gst_adapter_new (); self->started = FALSE; self->wait_for_keyframe = DEFAULT_WAIT_FOR_KEYFRAME; + self->last_pushed_was_lost_event = FALSE; } static void @@ -187,9 +188,36 @@ send_last_lost_event (GstRtpVP8Depay * self) ->packet_lost (GST_RTP_BASE_DEPAYLOAD_CAST (self), self->last_lost_event); gst_event_replace (&self->last_lost_event, NULL); + self->last_pushed_was_lost_event = TRUE; } } +static void +send_new_lost_event (GstRtpVP8Depay * self, GstClockTime timestamp, + guint new_picture_id, const gchar * reason) +{ + GstEvent *event; + + if (!GST_CLOCK_TIME_IS_VALID (timestamp)) { + GST_WARNING_OBJECT (self, "Can't create lost event with invalid timestmap"); + return; + } + + event = gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM, + gst_structure_new ("GstRTPPacketLost", + "timestamp", G_TYPE_UINT64, timestamp, + "duration", G_TYPE_UINT64, 0, NULL)); + + GST_DEBUG_OBJECT (self, "Pushing lost event " + "(picids 0x%x 0x%x, reason \"%s\"): %" GST_PTR_FORMAT, + self->last_picture_id, new_picture_id, reason, event); + + GST_RTP_BASE_DEPAYLOAD_CLASS (gst_rtp_vp8_depay_parent_class) + ->packet_lost (GST_RTP_BASE_DEPAYLOAD_CAST (self), event); + + gst_event_unref (event); +} + static void send_last_lost_event_if_needed (GstRtpVP8Depay * self, guint new_picture_id) { @@ -236,6 +264,10 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp) guint hdrsize = 1; guint picture_id = PICTURE_ID_NONE; guint size = gst_rtp_buffer_get_payload_len (rtp); + guint s_bit; + guint part_id; + gboolean frame_start; + gboolean sent_lost_event = FALSE; if (G_UNLIKELY (GST_BUFFER_IS_DISCONT (rtp->buffer))) { GST_DEBUG_OBJECT (self, "Discontinuity, flushing adapter"); @@ -251,6 +283,10 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp) goto too_small; data = gst_rtp_buffer_get_payload (rtp); + + s_bit = (data[0] >> 4) & 0x1; + part_id = (data[0] >> 0) & 0x7; + /* Check X optional header */ if ((data[0] & 0x80) != 0) { hdrsize++; @@ -275,18 +311,30 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp) if ((data[1] & 0x20) != 0 || (data[1] & 0x10) != 0) hdrsize++; } - GST_LOG_OBJECT (depay, "hdrsize %u, size %u, picture id 0x%x", hdrsize, - size, picture_id); + GST_LOG_OBJECT (depay, + "hdrsize %u, size %u, picture id 0x%x, s %u, part_id %u", hdrsize, size, + picture_id, s_bit, part_id); if (G_UNLIKELY (hdrsize >= size)) goto too_small; - if (G_UNLIKELY (!self->started)) { - /* Check if this is the start of a VP8 frame, otherwise bail */ - /* S=1 and PartID= 0 */ - if ((data[0] & 0x17) != 0x10) { + frame_start = (s_bit == 1) && (part_id == 0); + if (frame_start) { + if (G_UNLIKELY (self->started)) { + GST_DEBUG_OBJECT (depay, "Incomplete frame, flushing adapter"); + gst_adapter_clear (self->adapter); + self->started = FALSE; + + send_new_lost_event (self, GST_BUFFER_PTS (rtp->buffer), picture_id, + "Incomplete frame detected"); + sent_lost_event = TRUE; + } + } + + if (!self->started) { + if (G_UNLIKELY (!frame_start)) { GST_DEBUG_OBJECT (depay, - "The frame is missing the first packets, ignoring the packet"); - if (self->stop_lost_events) { + "The frame is missing the first packet, ignoring the packet"); + if (self->stop_lost_events && !sent_lost_event) { send_last_lost_event (self); self->stop_lost_events = FALSE; } @@ -295,7 +343,7 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp) GST_LOG_OBJECT (depay, "Found the start of the frame"); - if (self->stop_lost_events) { + if (self->stop_lost_events && !sent_lost_event) { send_last_lost_event_if_needed (self, picture_id); self->stop_lost_events = FALSE; } @@ -373,6 +421,9 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp) if (picture_id != PICTURE_ID_NONE) self->stop_lost_events = TRUE; + + self->last_pushed_was_lost_event = FALSE; + return out; } @@ -441,6 +492,8 @@ gst_rtp_vp8_depay_packet_lost (GstRTPBaseDepayload * depay, GstEvent * event) GstRtpVP8Depay *self = GST_RTP_VP8_DEPAY (depay); const GstStructure *s; gboolean might_have_been_fec; + gboolean unref_event = FALSE; + gboolean ret; s = gst_event_get_structure (event); @@ -453,16 +506,30 @@ gst_rtp_vp8_depay_packet_lost (GstRTPBaseDepayload * depay, GstEvent * event) return TRUE; } } else if (self->last_picture_id != PICTURE_ID_NONE) { - GstStructure *s = gst_event_writable_structure (self->last_lost_event); + GstStructure *s; + + if (!gst_event_is_writable (event)) { + event = gst_event_copy (event); + unref_event = TRUE; + } + + s = gst_event_writable_structure (event); /* We are currently processing a picture, let's make sure the * base depayloader doesn't drop this lost event */ gst_structure_remove_field (s, "might-have-been-fec"); } - return + self->last_pushed_was_lost_event = TRUE; + + ret = GST_RTP_BASE_DEPAYLOAD_CLASS (gst_rtp_vp8_depay_parent_class)->packet_lost (depay, event); + + if (unref_event) + gst_event_unref (event); + + return ret; } gboolean diff --git a/gst/rtp/gstrtpvp8depay.h b/gst/rtp/gstrtpvp8depay.h index c53ccaa8ea..5582d634f4 100644 --- a/gst/rtp/gstrtpvp8depay.h +++ b/gst/rtp/gstrtpvp8depay.h @@ -70,6 +70,7 @@ struct _GstRtpVP8Depay guint last_picture_id; gboolean wait_for_keyframe; + gboolean last_pushed_was_lost_event; }; GType gst_rtp_vp8_depay_get_type (void); diff --git a/tests/check/elements/rtpvp8.c b/tests/check/elements/rtpvp8.c index 44112898a3..939c0280a9 100644 --- a/tests/check/elements/rtpvp8.c +++ b/tests/check/elements/rtpvp8.c @@ -48,8 +48,8 @@ static guint8 intra_nopicid_seqnum0[] = { }; static GstBuffer * -create_rtp_vp8_buffer (guint seqnum, guint picid, gint picid_bits, - GstClockTime buf_pts) +create_rtp_vp8_buffer_full (guint seqnum, guint picid, gint picid_bits, + GstClockTime buf_pts, gboolean s_bit, gboolean marker_bit) { GstBuffer *ret; guint8 *packet; @@ -74,11 +74,29 @@ create_rtp_vp8_buffer (guint seqnum, guint picid, gint picid_bits, packet[2] = (seqnum >> 8) & 0xff; packet[3] = (seqnum >> 0) & 0xff; + if (marker_bit) + packet[1] |= 0x80; + else + packet[1] &= ~0x80; + + if (s_bit) + packet[12] |= 0x10; + else + packet[12] &= ~0x10; + ret = gst_buffer_new_wrapped (packet, size); GST_BUFFER_PTS (ret) = buf_pts; return ret; } +static GstBuffer * +create_rtp_vp8_buffer (guint seqnum, guint picid, guint picid_bits, + GstClockTime buf_pts) +{ + return create_rtp_vp8_buffer_full (seqnum, picid, picid_bits, buf_pts, TRUE, + TRUE); +} + static void add_vp8_meta (GstBuffer * buffer, gboolean use_temporal_scaling, gboolean layer_sync, guint layer_id, guint tl0picidx) @@ -502,6 +520,15 @@ typedef struct _DepayGapEventTestData gint picid_bits; } DepayGapEventTestData; +typedef struct +{ + gint seq_num; + gint picid; + guint buffer_type; + gboolean s_bit; + gboolean marker_bit; +} DepayGapEventTestDataFull; + static void test_depay_gap_event_base (const DepayGapEventTestData * data, gboolean send_lost_event, gboolean expect_gap_event) @@ -571,6 +598,153 @@ GST_START_TEST (test_depay_stop_gap_events) GST_END_TEST; +GST_START_TEST (test_depay_send_gap_event_when_marker_bit_missing_and_picid_gap) +{ + gboolean send_lost_event = __i__ == 0; + GstClockTime pts = 0; + gint seqnum = 100; + gint i; + GstEvent *event; + + GstHarness *h = gst_harness_new_parse ("rtpvp8depay"); + gst_harness_set_src_caps_str (h, RTP_VP8_CAPS_STR); + + /* Push a complete frame to avoid depayloader to suppress gap events */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 23, + 7, pts, TRUE, TRUE))); + pts += 33 * GST_MSECOND; + seqnum++; + + for (i = 0; i < 3; i++) + gst_event_unref (gst_harness_pull_event (h)); + fail_unless_equals_int (gst_harness_events_in_queue (h), 0); + + /* Push packet with start bit set, but no marker bit */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 24, + 7, pts, TRUE, FALSE))); + pts += 33 * GST_MSECOND; + seqnum++; + + if (send_lost_event) { + gst_harness_push_event (h, + gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM, + gst_structure_new ("GstRTPPacketLost", "timestamp", G_TYPE_UINT64, + pts, "duration", G_TYPE_UINT64, 33 * GST_MSECOND, NULL))); + pts += 33 * GST_MSECOND; + seqnum++; + } + + /* Push packet with gap in picid */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 26, + 7, pts, TRUE, TRUE))); + + /* Expect only 2 output frames since the one frame was incomplete */ + fail_unless_equals_int (2, gst_harness_buffers_received (h)); + + /* There should be a gap event, either triggered by the loss or the picid + * gap */ + + /* Making sure the GAP event was pushed downstream */ + event = gst_harness_pull_event (h); + fail_unless_equals_string ("gap", + gst_event_type_get_name (GST_EVENT_TYPE (event))); + gst_event_unref (event); + + fail_unless_equals_int (gst_harness_events_in_queue (h), 0); + + gst_harness_teardown (h); +} + +GST_END_TEST; + +GST_START_TEST + (test_depay_send_gap_event_when_marker_bit_missing_and_no_picid_gap) { + GstClockTime pts = 0; + gint seqnum = 100; + gint i; + GstEvent *event; + + GstHarness *h = gst_harness_new_parse ("rtpvp8depay"); + gst_harness_set_src_caps_str (h, RTP_VP8_CAPS_STR); + + /* Push a complete frame to avoid depayloader to suppress gap events */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 23, + 7, pts, TRUE, TRUE))); + pts += 33 * GST_MSECOND; + seqnum++; + + for (i = 0; i < 3; i++) + gst_event_unref (gst_harness_pull_event (h)); + fail_unless_equals_int (gst_harness_events_in_queue (h), 0); + + /* Push packet with start bit set, but no marker bit */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 24, + 7, pts, TRUE, FALSE))); + pts += 33 * GST_MSECOND; + seqnum++; + + /* Push packet for next picid, without having sent a packet with marker bit + * foor the previous picid */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 25, + 7, pts, TRUE, TRUE))); + + /* Expect only 2 output frames since one was incomplete */ + fail_unless_equals_int (2, gst_harness_buffers_received (h)); + + /* Making sure the GAP event was pushed downstream */ + event = gst_harness_pull_event (h); + gst_event_unref (event); + + fail_unless_equals_int (gst_harness_events_in_queue (h), 0); + + gst_harness_teardown (h); +} + +GST_END_TEST; + +GST_START_TEST (test_depay_no_gap_event_when_partial_frames_with_no_picid_gap) +{ + gint i; + GstClockTime pts = 0; + GstHarness *h = gst_harness_new ("rtpvp8depay"); + gst_harness_set_src_caps_str (h, RTP_VP8_CAPS_STR); + + /* start with complete frame to make sure depayloader will not drop + * potential gap events */ + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (100, 24, + 7, pts, TRUE, TRUE))); + fail_unless_equals_int (1, gst_harness_buffers_received (h)); + + /* drop setup events to more easily check for gap events */ + for (i = 0; i < 3; i++) + gst_event_unref (gst_harness_pull_event (h)); + fail_unless_equals_int (gst_harness_events_in_queue (h), 0); + + /* Next frame is split in two packets */ + pts += 33 * GST_MSECOND; + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (101, 25, + 7, pts, TRUE, FALSE))); + fail_unless_equals_int (GST_FLOW_OK, + gst_harness_push (h, create_rtp_vp8_buffer_full (102, 25, + 7, pts, FALSE, TRUE))); + fail_unless_equals_int (2, gst_harness_buffers_received (h)); + + /* there must be no gap events */ + fail_unless_equals_int (gst_harness_events_in_queue (h), 0); + + gst_harness_teardown (h); +} + +GST_END_TEST; + /* Packet loss + lost picture ids */ static const DepayGapEventTestData resend_gap_event_test_data[][2] = { /* 7bit picture ids */ @@ -617,6 +791,12 @@ rtpvp8_suite (void) G_N_ELEMENTS (stop_gap_events_test_data)); tcase_add_loop_test (tc_chain, test_depay_resend_gap_event, 0, G_N_ELEMENTS (resend_gap_event_test_data)); + tcase_add_loop_test (tc_chain, + test_depay_send_gap_event_when_marker_bit_missing_and_picid_gap, 0, 2); + tcase_add_test (tc_chain, + test_depay_send_gap_event_when_marker_bit_missing_and_no_picid_gap); + tcase_add_test (tc_chain, + test_depay_no_gap_event_when_partial_frames_with_no_picid_gap); return s; }