| Message ID | 20190331013233.11381-1-david.holroyd@m2amedia.tv |
|---|---|
| State | New |
| Headers | show |
On Sun, Mar 31, 2019 at 02:32:34AM +0100, David Holroyd wrote: > Previously, prompeg_write() would only report to caller that bytes we > written when a FEC packet was actually created. Not all RTP packets are > expected to generate a FEC packet however, so this behavior was causing > avio to retry writing the RTP packet, eventually forcing the FEC state > machine to send a FEC packet erroneously (and so breaking out of the > retry loop). > > This was resulting in incorrect FEC data being generated, and far too > many FEC packets to be sent (~100% FEC overhead). > --- > libavformat/prompeg.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) this patch doesnt seem to apply automatically with git am also no fate test changes, which indicates that this code is not tested. I think it would be rather useful if there was a test that covers this, otherwise any future change to thsís code could break it with the person noticing thanks [...]
On Tue, Apr 2, 2019 at 5:20 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sun, Mar 31, 2019 at 02:32:34AM +0100, David Holroyd wrote: > > Previously, prompeg_write() would only report to caller that bytes we > > written when a FEC packet was actually created. Not all RTP packets are > > expected to generate a FEC packet however, so this behavior was causing > > avio to retry writing the RTP packet, eventually forcing the FEC state > > machine to send a FEC packet erroneously (and so breaking out of the > > retry loop). > > > > This was resulting in incorrect FEC data being generated, and far too > > many FEC packets to be sent (~100% FEC overhead). > > --- > > libavformat/prompeg.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > this patch doesnt seem to apply automatically with git am > > also no fate test changes, which indicates that this code is not tested. > I think it would be rather useful if there was a test that covers this, > otherwise any future change to thsís code could break it with the person > noticing > > thanks It's about issue http://trac.ffmpeg.org/ticket/7863, and I can confirm this fix is working (with wireshark), can we merge this patch first? I will add the FATE testcase.
On Mon, Jun 08, 2020 at 04:56:04PM +0800, mypopy@gmail.com wrote: > On Tue, Apr 2, 2019 at 5:20 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sun, Mar 31, 2019 at 02:32:34AM +0100, David Holroyd wrote: > > > Previously, prompeg_write() would only report to caller that bytes we > > > written when a FEC packet was actually created. Not all RTP packets are > > > expected to generate a FEC packet however, so this behavior was causing > > > avio to retry writing the RTP packet, eventually forcing the FEC state > > > machine to send a FEC packet erroneously (and so breaking out of the > > > retry loop). > > > > > > This was resulting in incorrect FEC data being generated, and far too > > > many FEC packets to be sent (~100% FEC overhead). > > > --- > > > libavformat/prompeg.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > this patch doesnt seem to apply automatically with git am > > > > also no fate test changes, which indicates that this code is not tested. > > I think it would be rather useful if there was a test that covers this, > > otherwise any future change to thsís code could break it with the person > > noticing > > > > thanks > It's about issue http://trac.ffmpeg.org/ticket/7863, and I can confirm > this fix is working (with wireshark), can we merge this patch first? I > will add the FATE testcase. as the patch did not apply with git am, no it cannot be applied obviously thx [...]
On Mon, Jun 8, 2020 at 7:03 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Jun 08, 2020 at 04:56:04PM +0800, mypopy@gmail.com wrote: > > On Tue, Apr 2, 2019 at 5:20 AM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Sun, Mar 31, 2019 at 02:32:34AM +0100, David Holroyd wrote: > > > > Previously, prompeg_write() would only report to caller that bytes we > > > > written when a FEC packet was actually created. Not all RTP packets are > > > > expected to generate a FEC packet however, so this behavior was causing > > > > avio to retry writing the RTP packet, eventually forcing the FEC state > > > > machine to send a FEC packet erroneously (and so breaking out of the > > > > retry loop). > > > > > > > > This was resulting in incorrect FEC data being generated, and far too > > > > many FEC packets to be sent (~100% FEC overhead). > > > > --- > > > > libavformat/prompeg.c | 6 ++---- > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > this patch doesnt seem to apply automatically with git am > > > > > > also no fate test changes, which indicates that this code is not tested. > > > I think it would be rather useful if there was a test that covers this, > > > otherwise any future change to thsís code could break it with the person > > > noticing > > > > > > thanks > > It's about issue http://trac.ffmpeg.org/ticket/7863, and I can confirm > > this fix is working (with wireshark), can we merge this patch first? I > > will add the FATE testcase. > > as the patch did not apply with git am, no it cannot be applied obviously > > thx Will refine & re-submite the patch, thx
diff --git a/libavformat/prompeg.c b/libavformat/prompeg.c index 94b556d5f1..c3bd5f1249 100644 --- a/libavformat/prompeg.c +++ b/libavformat/prompeg.c @@ -387,7 +387,7 @@ static int prompeg_write(URLContext *h, const uint8_t *buf, int size) { PrompegContext *s = h->priv_data; uint8_t *bitstring = NULL; int col_idx, col_out_idx, row_idx; - int ret, written = 0; + int ret = 0; if (s->init && ((ret = prompeg_init(h, buf, size)) < 0)) goto end; @@ -403,7 +403,6 @@ static int prompeg_write(URLContext *h, const uint8_t *buf, int size) { if (!s->first || s->packet_idx > 0) { if ((ret = prompeg_write_fec(h, s->fec_row, PROMPEG_FEC_ROW)) < 0) goto end; - written += ret; } memcpy(s->fec_row->bitstring, bitstring, s->bitstring_size); s->fec_row->sn = AV_RB16(buf + 2); @@ -431,7 +430,6 @@ static int prompeg_write(URLContext *h, const uint8_t *buf, int size) { col_out_idx = s->packet_idx / s->d; if ((ret = prompeg_write_fec(h, s->fec_col[col_out_idx], PROMPEG_FEC_COL)) < 0) goto end; - written += ret; } if (++s->packet_idx >= s->packet_idx_max) { @@ -440,7 +438,7 @@ static int prompeg_write(URLContext *h, const uint8_t *buf, int size) { s->first = 0; } - ret = written; + ret = size; end: av_free(bitstring);