On Wed, 25 Jul 2012, Samuel Pitoiset wrote:

On Wed, Jul 25, 2012 at 11:00 PM, Martin Storsjö <[email protected]> wrote:
On Wed, 25 Jul 2012, Samuel Pitoiset wrote:

On Wed, Jul 25, 2012 at 8:05 PM, Martin Storsjö <[email protected]> wrote:

On Sat, 21 Jul 2012, Samuel Pitoiset wrote:

---
libavformat/rtmpproto.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index c003b37..5af03c4 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -859,11 +859,11 @@ static int handle_client_bw(URLContext *s,
RTMPPacket *pkt)
{
    RTMPContext *rt = s->priv_data;

-    if (pkt->data_size < 4) {
+    if (pkt->data_size != 5) {
        av_log(s, AV_LOG_ERROR,
-               "Client bandwidth report packet is less than 4 bytes
long
(%d)\n",
+               "Client bandwidth packet is not 5 bytes long (%d)\n",
               pkt->data_size);
-        return -1;
+        return AVERROR(EINVAL);
    }

    rt->client_report_size = AV_RB32(pkt->data);
--
1.7.11.1



This changes behaviour - is there a reason to treat it as an error if the
client bandwidth packet is longer than 5 bytes? I'd rather have the error
code change separate from the behaviour change, and an explanation why
you'd
like to change the behaviour.


We already return an error code when a client bandwidth packet is less
than 4 bytes long (ie. it's more or less the same behaviour for chunk
size packets). So, I made these changes in order to be consistent
regarding the other handle functions I have refactored. Otherwise, I
submitted a separate patch which change the error code.


Ok, if the reason is to make things more consistent, that is a valid reason
I guess. If sent as a separate change from the other things with proper
explanation and motivation, I guess it might be ok,

Okay, I'll submit this change in a separate patch.

but then I'd rather have
the check in handle_chunk_size fixed instead - I'd rather be more liberal
when it comes to unknown data in packets.

We already check the packet size in handle_chunk_size, so what do you mean here?

Yes, please read again. Or in Simple English:

In handle_chunk_size we check data_size != 4, in handle_client_bw we check data_size < 4. Making these two consistent would be good. Then we should choose to use either < or != at both places. You tried to change it to != at both places. I prefer having < instead, since that allows having more unknown data at the end of the packet without failing, which is being liberal about what we accept.

// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to