Created attachment 338092 [details] [review]
Patch

Comment on attachment 338092 [details] [review]
Patch

There is a signal where applications can add new packets to the RTCP packet, which is called right before the warning you're concerned about.

It is completely valid to get here without any "internal" reason and let the application add some packets. That's what happens when the application would request an RTCP packet via the signal.

Oh, I see, so we have to allow an empty buffer is created in the queue and just discard after that, in that case can downgrade the log message to DEBUG?

But why is a empty buffer created here? This can AFAIU only happen if the application requests an RTCP packet but does not add any data to it. Is that what happens here?

Otherwise it would seem like a lurking bug in rtpsession somewhere. It should only schedule a packet if there is something to send (or if the application asks for it).

The reason is a source SSRC source needs to send a NACK, but we have multiple sources in that session, so we try to generate a packet for all of them but just one source has the send_nack to TRUE. The other ones are in early timeout and reduced situation so they generate the empty packet, for all the other SSRCs in the session. Does it make sense?

Actually, regarding the patch, we are not disabling users to use that signal, we are just not launching it for this specific case, which is not requested messages for an SSRC where we are not going to send a message, users could always send messages if we are going to send it, or just wait for the next expected timeout. And in the not early cases it is always because we are adding RR and SR. So maybe can we reconsider the orinal patch?

No, the patch prevents applications from appending more RTCP packets for a source (that has nothing else pending) via the on-sending-rtcp signal. The application would have to wait for the next regular schedule again (or an early schedule with actual packets for this specific source).


But I see what the problem is now. RTCP is scheduled per session, not per source. And some sources in the session might not have anything to report and thus an empty packet.

An alternative patch would make sense then, that moves the warning out of the loop. And only warns if *none* of the sources had a non-empty packet (i.e. if the whole session was scheduled without need). Does that make sense to you?

(In reply to Sebastian Dröge (slomo) from comment #7)
> But I see what the problem is now. RTCP is scheduled per session, not per
> source. And some sources in the session might not have anything to report
> and thus an empty packet.
>
> An alternative patch would make sense then, that moves the warning out of
> the loop. And only warns if *none* of the sources had a non-empty packet
> (i.e. if the whole session was scheduled without need). Does that make sense
> to you?

Yep, I think it is a good option too.

Thanks!

Great, are you going to provide a patch? :)

Yeah, I'm doing it, but there is one problem with code reorder because the done tag can be reached if we bailed completely of the generation, and adding code outside the while we have to be careful we do not cause new errors for that situation.

Created attachment 338187 [details] [review]
Patch

Review of attachment 338187 [details] [review]:

::: gst/rtpmanager/rtpsession.c
@@ +4070,3 @@
+  if (g_queue_is_empty (&data.output)) {
+    GST_ERROR ("rtpsession: No RTCP packet generated for a timeout");
+  }

That's not enough: there will always be buffers in the queue if there are sources. They just might be empty buffers.

@@ -4087,3 @@
 
-    if (empty_buffer)
-      GST_ERROR ("rtpsession: Trying to send an empty RTCP packet");

I would suggest to add a gboolean "all_empty" outside the loop and initialize it to TRUE. Once you find a non-empty buffer, you set it to FALSE. If it's TRUE after the loop, do a warning

(In reply to Sebastian Dröge (slomo) from comment #12)
> Review of attachment 338187 [details] [review] [review]:
> 
> ::: gst/rtpmanager/rtpsession.c
> @@ +4070,3 @@
> +  if (g_queue_is_empty (&data.output)) {
> +    GST_ERROR ("rtpsession: No RTCP packet generated for a timeout");
> +  }
> 
> That's not enough: there will always be buffers in the queue if there are
> sources. They just might be empty buffers.
>

Ok, I didn't understand your point initially.

> @@ -4087,3 @@
>  
> -    if (empty_buffer)
> -      GST_ERROR ("rtpsession: Trying to send an empty RTCP packet");
> 
> I would suggest to add a gboolean "all_empty" outside the loop and
> initialize it to TRUE. Once you find a non-empty buffer, you set it to
> FALSE. If it's TRUE after the loop, do a warning

Ok, I'll do that, thanks.

Created attachment 338336 [details] [review]
Patch

Review of attachment 338336 [details] [review]:

Looks generally good, thanks :)

::: gst/rtpmanager/rtpsession.c
@@ +3973,3 @@
   GHashTable *table_copy;
   ReportOutput *output;
+  gboolean all_empty = FALSE;

Why initialize to FALSE...

@@ +4038,3 @@
 
+  /* check if all the buffers are empty afer generation */
+  all_empty = TRUE;

...and then always set to TRUE here? (Also typo: afer)

(In reply to Sebastian Dröge (slomo) from comment #15)
> Review of attachment 338336 [details] [review] [review]:
> 
> Looks generally good, thanks :)
> 
> ::: gst/rtpmanager/rtpsession.c
> @@ +3973,3 @@
>    GHashTable *table_copy;
>    ReportOutput *output;
> +  gboolean all_empty = FALSE;
> 
> Why initialize to FALSE...
> 
> @@ +4038,3 @@
>  
> +  /* check if all the buffers are empty afer generation */
> +  all_empty = TRUE;
> 
> ...and then always set to TRUE here? (Also typo: afer)

The if before that line goes directly to done depending on the time, we do not want to launch and error in that case because we basically avoided the generation. The if is:

  /* see if we need to generate SR or RR packets */
  if (!is_rtcp_time (sess, current_time, &data))
    goto done;

commit 6e7816c589abd3f04eafa04694cc6ccb8943f7a3
Author: Alejandro G. Castro <alex@igalia.com>
Date:   Thu Oct 20 13:14:13 2016 +0200

    rtpbin: avoid generating errors when rtcp messages are empty and check the queue is not empty
    
    Add a check to verify all the output buffers were empty for the
    session in a timout and log an error.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=773269