GNOME Bugzilla – Bug 739868
rtpmanager: rtpjitterbuffer fixes and improvements
Last modified: 2015-08-16 13:38:24 UTC
I propose a set of patches to fix some bugs and to improve rtpmanager stack to work in real networks. I have explained each patch, but if you require it, I will explain them on demand.
Created attachment 290312 [details] [review] 0001-rtpjitterbuffer-implement-get-set-for-new-rtx-min-re.patch
Created attachment 290313 [details] [review] 0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch
Created attachment 290314 [details] [review] 0003-gstrtpjitterbuffer-fix-expected_dts-calc-in-calculat.patch
Created attachment 290315 [details] [review] 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
Created attachment 290316 [details] [review] 0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch
Created attachment 290317 [details] [review] 0006-gstrtpjitterbuffer-detect-disconts-before-a-buffer-i.patch
Created attachment 290318 [details] [review] 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Created attachment 290319 [details] [review] 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
Created attachment 290320 [details] [review] 0009-gstrtpjitterbuffer-add-rtx-max-retries-property.patch
Created attachment 290321 [details] [review] 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
Comment on attachment 290312 [details] [review] 0001-rtpjitterbuffer-implement-get-set-for-new-rtx-min-re.patch This patch is already merged into master branch.
Comment on attachment 290313 [details] [review] 0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch Related to https://bugzilla.gnome.org/show_bug.cgi?id=739344
Created attachment 290483 [details] [review] 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
Comment on attachment 290313 [details] [review] 0002-gstrtpjitterbuffer-ensure-rtx_retry_period-0.patch Was pushed ages ago: commit 6daa57868f01da46aa2c6cfc0a4dc8344312e4c1 Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Mon Oct 20 13:00:37 2014 +0200 rtpjitterbuffer: ensure rtx_retry_period >= 0 https://bugzilla.gnome.org/show_bug.cgi?id=739344
Comment on attachment 290316 [details] [review] 0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch There seems to be a real problem here but instead of increasing the number we should only reset when we're supposed to: i.e. not when a) we lost 100 packets in a row, or b) got a rtx packet with a seqnum gap >100. Resetting the stats in these two cases seems wrong. Question now is how we can distinguish between these cases, and the case when we actually want to reset the stats (because we think a new session started?)
Created attachment 299971 [details] [review] rtpjitterbuffer: Make the next output buffer discont after resetting the jitterbuffer Resetting the jitterbuffer drops all packets and other things, and will cause a discontinuity in the packets received by the depayloaders. They should now also flush anything they had pending as the new data will start at a different position.
Comment on attachment 290317 [details] [review] 0006-gstrtpjitterbuffer-detect-disconts-before-a-buffer-i.patch I don't think this patch is correct. 1) The gap event is wrong. It needs a valid timestamp and duration. 2) not resetting last_popped_seqnum might break other things The patch is just attached here to always set discont after resetting the jitterbuffer should solve all the problems you saw here. Can you test and confirm?
Comment on attachment 290320 [details] [review] 0009-gstrtpjitterbuffer-add-rtx-max-retries-property.patch Makes sense to me
Comment on attachment 290483 [details] [review] 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch This looks good and also makes sense to me. Only the documentation needs to be a bit more clear :) I would just do that when merging it. What this does is that setting the property to FALSE will cause it to not estimate when the next packet should arrive and start a retransmission request timer for it. It will still start retransmission request timers if a gap in sequence numbers is detected, just not for future packets before we noticed a gap. Especially we won't request a retransmission now if the sender is just not sending us any packets for some time (e.g. someone just switched off the camera for a while ;) ).
Review of attachment 290315 [details] [review]: This looks correct to me. Adding the retransmissions to the jitter will make it wrong as the rtptime of the retransmissions will be very much in the past (we wanted to have that packet earlier already, right?) but the receive time was just now. ::: gst/rtpmanager/gstrtpjitterbuffer.c @@ +2213,3 @@ + if (G_LIKELY (gap > -(priv->rtx_delay_reorder))) { + calculate_jitter (jitterbuffer, dts, rtptime); This won't do the right thing if rtx_delay_reorder==-1 ("automatic"). In that case we would ignore all expected packets (gap==0) and also all packets that are one in the future from our expected packet. With rtx_delay_reorder==0 we would also ignore all expected packets. Otherwise this check seems correct.
Comment on attachment 290319 [details] [review] 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch I don't think this is correct. We also add timers for future packets (do_next_seqnum), and add timers for packets for which we just have reordering (i.e. a short gap). So assuming that all timers are lost packets is not correct, and this will probably cause the jitterbuffer to reset itself too early.
Comment on attachment 290314 [details] [review] 0003-gstrtpjitterbuffer-fix-expected_dts-calc-in-calculat.patch I think this is also correct. Right above we consider lost_packet packets, each of them having duration, as lost and triggered their timers immediately... below we use expected_dts to schedule retransmission or schedule lost timers for the packets that come after expected_dts. As we just triggered lost_packets packets as lost, there's no point in scheduling new timers for them and we can just skip over all lost packets.
Comment on attachment 290318 [details] [review] 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch This changes the default behaviour from 3000 as rtp-max-dropout to something adaptive. Not sure that's a good idea, the default should probably stay as is. Then calculate_n_packets_last_second() returns the length of the queue but you don't use that. You don't need it? destroy_GstClockTime() should be renamed to something non-camelcase :) clock_time_free() or something. get_rtp_max_dropout() seems to do the wrong thing if we have less than 1 second of packets so far. If we have more than 1 second it will amount to about 0.5s worth of data, if we have less than 1 second (e.g. X=0.5 seconds), we will only allow 0.25 (X/2) of dropout. I think this code should just return some default if we didn't observe 1 second of packets yet. I think the dts queue also needs to be reset whenever the jitterbuffer is reset, otherwise it will contain old data that is not relevant anymore.
Review of attachment 290317 [details] [review]: Replaced by https://bugzilla.gnome.org/review?bug=739868&attachment=299971
(In reply to Sebastian Dröge (slomo) from comment #23) > Comment on attachment 290318 [details] [review] [review] > 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch > > This changes the default behaviour from 3000 as rtp-max-dropout to something > adaptive. Not sure that's a good idea, the default should probably stay as > is. > By default this property has RTP_MAX_DROPOUT (3000) as value, so it is not adaptative because get_rtp_max_dropout returns this value. > Then calculate_n_packets_last_second() returns the length of the queue but > you don't use that. You don't need it? > This function returns the length if the user want to use it. In my case I don't use this returned value but use the length of the queue into get_rtp_max_dropout, so I change it to returns void. > destroy_GstClockTime() should be renamed to something non-camelcase :) > clock_time_free() or something. > Done! > get_rtp_max_dropout() seems to do the wrong thing if we have less than 1 > second of packets so far. If we have more than 1 second it will amount to > about 0.5s worth of data, if we have less than 1 second (e.g. X=0.5 > seconds), we will only allow 0.25 (X/2) of dropout. I think this code should > just return some default if we didn't observe 1 second of packets yet. > Done! > > I think the dts queue also needs to be reset whenever the jitterbuffer is > reset, otherwise it will contain old data that is not relevant anymore. Done!
(In reply to Sebastian Dröge (slomo) from comment #20) > Review of attachment 290315 [details] [review] [review]: > > This looks correct to me. Adding the retransmissions to the jitter will make > it wrong as the rtptime of the retransmissions will be very much in the past > (we wanted to have that packet earlier already, right?) but the receive time > was just now. > > ::: gst/rtpmanager/gstrtpjitterbuffer.c > @@ +2213,3 @@ > > + if (G_LIKELY (gap > -(priv->rtx_delay_reorder))) { > + calculate_jitter (jitterbuffer, dts, rtptime); > > This won't do the right thing if rtx_delay_reorder==-1 ("automatic"). In > that case we would ignore all expected packets (gap==0) and also all packets > that are one in the future from our expected packet. With > rtx_delay_reorder==0 we would also ignore all expected packets. > > Otherwise this check seems correct. Done!
(In reply to Sebastian Dröge (slomo) from comment #21) > Comment on attachment 290319 [details] [review] [review] > 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch > > I don't think this is correct. We also add timers for future packets > (do_next_seqnum), and add timers for packets for which we just have > reordering (i.e. a short gap). So assuming that all timers are lost packets > is not correct, and this will probably cause the jitterbuffer to reset > itself too early. Fixed!
Created attachment 300123 [details] [review] 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Created attachment 300124 [details] [review] 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
Created attachment 300125 [details] [review] 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
(In reply to Sebastian Dröge (slomo) from comment #15) > Comment on attachment 290316 [details] [review] [review] > 0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch > > There seems to be a real problem here but instead of increasing the number > we should only reset when we're supposed to: i.e. not when a) we lost 100 > packets in a row, or b) got a rtx packet with a seqnum gap >100. > Resetting the stats in these two cases seems wrong. > > > Question now is how we can distinguish between these cases, and the case > when we actually want to reset the stats (because we think a new session > started?) Could we accept this patch for the moment and then think about a better solution?
Created attachment 300185 [details] [review] 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch
Created attachment 300186 [details] [review] 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
Created attachment 300187 [details] [review] 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch
Comment on attachment 290483 [details] [review] 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch This one does not apply to latest git master anymore, please update it :) And while at it, please also improve the documentation a bit to be more descriptive. See my other comment about this (comment 19)
Created attachment 301447 [details] [review] 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch
(In reply to Miguel París Díaz from comment #31) > (In reply to Sebastian Dröge (slomo) from comment #15) > > Comment on attachment 290316 [details] [review] [review] [review] > > 0005-rtpstats-RTP_MAX_MISORDER-as-3000.patch > > > > There seems to be a real problem here but instead of increasing the number > > we should only reset when we're supposed to: i.e. not when a) we lost 100 > > packets in a row, or b) got a rtx packet with a seqnum gap >100. > > Resetting the stats in these two cases seems wrong. > > > > > > Question now is how we can distinguish between these cases, and the case > > when we actually want to reset the stats (because we think a new session > > started?) > > Could we accept this patch for the moment and then think about a better > solution? No, but I think I have a reproducible testcase for this now... and also think I know what the problem is. The fix will only work properly if you use the rtxsend/rtxreceive elements, not rtxqueue as you currently do (which is also not conforming to the RFC).
See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug. Please confirm that this is what you're seeing.
(In reply to Miguel París Díaz from comment #36) > Created attachment 301447 [details] [review] [review] > 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch For this one, also see bug #748041
(In reply to Sebastian Dröge (slomo) from comment #38) > See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and > https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug. > Please confirm that this is what you're seeing. The main problem is that RTCP stats are reset into rtpsource (https://bugzilla.gnome.org/review?bug=739868&attachment=290316). The point is why RTP_MAX_MISORDER is 100, and not 500, 2000 or 3000? Moreover, if the solution is based on RFC4588 (using rtprtxreceive and rtprtxsend) we will keep the error if the peer does not support this RFC.
(In reply to Sebastian Dröge (slomo) from comment #39) > (In reply to Miguel París Díaz from comment #36) > > Created attachment 301447 [details] [review] [review] [review] > > 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch > > For this one, also see bug #748041 Your description is very good and matches with my observations. What do you think about providing the patch to the new bug?
(In reply to Miguel París Díaz from comment #40) > (In reply to Sebastian Dröge (slomo) from comment #38) > > See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and > > https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug. > > Please confirm that this is what you're seeing. > > The main problem is that RTCP stats are reset into rtpsource > (https://bugzilla.gnome.org/review?bug=739868&attachment=290316). > > The point is why RTP_MAX_MISORDER is 100, and not 500, 2000 or 3000? Because even 100 is already very unlikely to be packet reordering. Changing that will only hide other problems, not solve anything. Your problem is not packet reordering I think, but delayed RTX. See the other bug :) We should fix that better somehow than changing some magic number. > Moreover, if the solution is based on RFC4588 (using rtprtxreceive and > rtprtxsend) we will keep the error if the peer does not support this RFC. Well, you should not enable RTX if the peer does not support it.
(In reply to Sebastian Dröge (slomo) from comment #42) > (In reply to Miguel París Díaz from comment #40) > > (In reply to Sebastian Dröge (slomo) from comment #38) > > > See https://bugzilla.gnome.org/show_bug.cgi?id=747922 and > > > https://bugzilla.gnome.org/show_bug.cgi?id=747919 for that specific bug. > > > Please confirm that this is what you're seeing. > > > > The main problem is that RTCP stats are reset into rtpsource > > (https://bugzilla.gnome.org/review?bug=739868&attachment=290316). > > > > The point is why RTP_MAX_MISORDER is 100, and not 500, 2000 or 3000? > > Because even 100 is already very unlikely to be packet reordering. Changing > that will only hide other problems, not solve anything. > > Your problem is not packet reordering I think, but delayed RTX. See the > other bug :) We should fix that better somehow than changing some magic > number. > Yes, I agree with you and I also think that it is a better solution. > > Moreover, if the solution is based on RFC4588 (using rtprtxreceive and > > rtprtxsend) we will keep the error if the peer does not support this RFC. > > Well, you should not enable RTX if the peer does not support it. The point here is that there are some clients that supports NACK but do not implement RFC4588. So we should take into account this.
(In reply to Miguel París Díaz from comment #43) > > > Moreover, if the solution is based on RFC4588 (using rtprtxreceive and > > > rtprtxsend) we will keep the error if the peer does not support this RFC. > > > > Well, you should not enable RTX if the peer does not support it. > The point here is that there are some clients that supports NACK but do not > implement RFC4588. So we should take into account this. What do they implement then, how does it work, how is it signalled in the SDP and is there an RFC for that?
One of this client is Firefox (WebRtc). It uses the same SSRC for original-RTP and rtx-RTP. An SDP example: v=0 o=mozilla...THIS_IS_SDPARTA-37.0.1 772883292565269152 0 IN IP4 0.0.0.0 s=- t=0 0 a=fingerprint:sha-256 98:EE:73:69:EC:81:FB:57:FF:19:03:20:5E:1E:4F:5E:CE:A3:6D:80:84:40:ED:E2:A4:47:78:AF:4E:27:56:0F a=group:BUNDLE sdparta_0 sdparta_1 a=ice-options:trickle m=audio 9 RTP/SAVPF 109 9 0 8 c=IN IP4 0.0.0.0 a=sendrecv a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level a=ice-pwd:383e0361d8982a7f5f8d461a58c4ac2c a=ice-ufrag:1d20324c a=mid:sdparta_0 a=rtcp-mux a=rtpmap:109 opus/48000/2 a=rtpmap:9 G722/8000/1 a=rtpmap:0 PCMU/8000 a=rtpmap:8 PCMA/8000 a=setup:actpass a=ssrc:1032149603 cname:{17f63fe4-1f23-4bce-b38f-f41d487ebaeb} m=video 9 RTP/SAVPF 120 126 97 c=IN IP4 0.0.0.0 a=sendrecv a=fmtp:120 max-fs=12288;max-fr=60 a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1 a=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1 a=ice-pwd:383e0361d8982a7f5f8d461a58c4ac2c a=ice-ufrag:1d20324c a=mid:sdparta_1 a=rtcp-fb:120 nack a=rtcp-fb:120 nack pli a=rtcp-fb:120 ccm fir a=rtcp-fb:126 nack a=rtcp-fb:126 nack pli a=rtcp-fb:126 ccm fir a=rtcp-fb:97 nack a=rtcp-fb:97 nack pli a=rtcp-fb:97 ccm fir a=rtcp-mux a=rtpmap:120 VP8/90000 a=rtpmap:126 H264/90000 a=rtpmap:97 H264/90000 a=setup:actpass a=ssrc:3142525711 cname:{17f63fe4-1f23-4bce-b38f-f41d487ebaeb}
Ok, but it uses a different PT. That's not ideal but AFAIU also works with rtxreceive/rtxsend and also seems to be "allowed" by the RFC (and discouraged). Just curious but does it also require us to send RTX packets with the same SSRC? Also even if Firefox would send us RTX packets with the same SSRC and PT, it should work just fine. FWIW, I'm fairly sure that Firefox' WebRTC stuff works fine with our RTX. I think it was tested for OpenWebRTC (which only uses rtxsend/receive).
Oh nevermind, that SDP is not even RTX if I read that correctly.
Does Firefox really retransmit packets if you send a generic NACK? That seems to be not defined by any RFC I read so far. RFC4585 for example does not define any semantics at all for that: https://tools.ietf.org/html/rfc4585#section-6.2.1 (it does define them only in combination with e.g. PLI)
Basically what I mean is that if I understand this correctly this SDP only tells us that we can send generic NACKs... and then something may happen or not. And if you on the sender side add an rtxqueue or not, is really your decision. Nothing is required at all here, you only say you can receive generic NACKs (and maybe you use them only for statistical purposes). Anyway, rtxqueue is still going to work. The latest plans for fixing all this are independent of which elements are used.
Comment on attachment 301447 [details] [review] 0010-gstrtpjitterbuffer-add-rtx-next-seqnum-property.patch commit f81c9a956835247b02ab69509021cb39446840ad Author: Miguel París Díaz <mparisdiaz@gmail.com> Date: Mon Apr 13 11:20:40 2015 +0200 rtpjitterbuffer: Add "rtx-next-seqnum" property If this is set to FALSE, rtpjitterbuffer will not request retransmissions for future packets based on when they are estimated to arrive. See also https://bugzilla.gnome.org/show_bug.cgi?id=748041 https://bugzilla.gnome.org/show_bug.cgi?id=739868
Comment on attachment 300185 [details] [review] 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch Wim said on IRC that we should handle all packets the same here
Comment on attachment 300186 [details] [review] 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch This does not apply to git master anymore, it looks like you accidentially merged another patch in here that is merged in master already.
(In reply to Sebastian Dröge (slomo) from comment #48) > Does Firefox really retransmit packets if you send a generic NACK? That > seems to be not defined by any RFC I read so far. RFC4585 for example does > not define any semantics at all for that: > https://tools.ietf.org/html/rfc4585#section-6.2.1 (it does define them only > in combination with e.g. PLI) Hello Sebastian, I have done some tests with Firefox and I can confirm that it retransmits the packets if I request them using generic NACKs. So, we should take into account this case when applying the solutions about we are thinking.
(In reply to Sebastian Dröge (slomo) from comment #51) > Comment on attachment 300185 [details] [review] [review] > 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch > > Wim said on IRC that we should handle all packets the same here I don't agree, because of the case of handling RTX packets(the common problem that we are discussing in other patches). If this is not a good solution, we should look for another one.
(In reply to Miguel París Díaz from comment #53) > (In reply to Sebastian Dröge (slomo) from comment #48) > > Does Firefox really retransmit packets if you send a generic NACK? That > > seems to be not defined by any RFC I read so far. RFC4585 for example does > > not define any semantics at all for that: > > https://tools.ietf.org/html/rfc4585#section-6.2.1 (it does define them only > > in combination with e.g. PLI) > > I have done some tests with Firefox and I can confirm that it retransmits > the packets if I request them using generic NACKs. > So, we should take into account this case when applying the solutions about > we are thinking. Sure, there's currently nothing RTX specific planned at all for these things. Nonetheless what Firefox is doing there is completely undefined and it would be better if they would actually implement the standard for that. And in the case of Firefox you really have no way to distinguish RTX from normal transmissions. If the RFC is implemented, you can distinguish them based on the PT and SSRC, like one of my patches in the other bugs did (which was not the best solution for that problem, but maybe it is for another one). (In reply to Miguel París Díaz from comment #54) > (In reply to Sebastian Dröge (slomo) from comment #51) > > Comment on attachment 300185 [details] [review] [review] [review] > > 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch > > > > Wim said on IRC that we should handle all packets the same here > > I don't agree, because of the case of handling RTX packets(the common > problem that we are discussing in other patches). > If this is not a good solution, we should look for another one. How does this patch relate to any of the others? Wim's argument basically was that there is nothing else we can do than to consider RTX packets also for the jitter calculations. And it might also be just jitter in the end. What exactly would your patch solve? What's the problem you're observing?
Created attachment 302253 [details] [review] 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
(In reply to Sebastian Dröge (slomo) from comment #55) > (In reply to Miguel París Díaz from comment #54) > > (In reply to Sebastian Dröge (slomo) from comment #51) > > > Comment on attachment 300185 [details] [review] [review] [review] [review] > > > 0004-gstrtpjitterbuffer-not-calculate-jitter-for-packets-.patch > > > > > > Wim said on IRC that we should handle all packets the same here > > > > I don't agree, because of the case of handling RTX packets(the common > > problem that we are discussing in other patches). > > If this is not a good solution, we should look for another one. > > How does this patch relate to any of the others? I mean that jitterbuffer has problems if manage RTX packets as the "original" packets. We have been discussing about how to avoid this case using RFC4588, using other PT and/or SSRC in the RTX packets, but we have to take into account that there are clients (as Firefox) that do no do this correctly. So: - Could we do something to do GStreamer works fine in these cases? - Should we assume this case and keep the current behaviour? > Wim's argument basically > was that there is nothing else we can do than to consider RTX packets also > for the jitter calculations. And it might also be just jitter in the end. > What exactly would your patch solve? What's the problem you're observing? This patch aims to avoid using RTX packets for jitter calculation because it is done basing on the last received packet, and this is not real.
Hello again Sebastian, what do you thing about the new patches?: 0008-gstrtpjitterbuffer-consider-timers-len-to-compare-wi.patch 0007-gstrtpjitterbuffer-add-rtp-max-dropout-property.patch
I didn't have time yet to think about them again, sorry. They're still on my list though. But also see the linked bugs, that might solve your problems better and some feedback from you there would be useful.
Miguel, can you test if the changes in bug #747922 are helping with your problems (while not using your max-dropout patch).
Hello Sebastian, I have to adapt my testing tools and reproduce losses scenarios and so on to check it. I hope do it as soon as I have a time slot.
Review of attachment 300187 [details] [review]: Rejected in favour this: https://bugzilla.gnome.org/show_bug.cgi?id=751636
Review of attachment 302253 [details] [review]: Rejected in favour this: https://bugzilla.gnome.org/show_bug.cgi?id=751311
Review of attachment 290316 [details] [review]: Rejected in favour of this: https://bugzilla.gnome.org/show_bug.cgi?id=751311