On Wed, Jul 25, 2012 at 11:15 PM, Martin Storsjö <mar...@martin.st> wrote:
> On Wed, 25 Jul 2012, Samuel Pitoiset wrote:
>
>> On Wed, Jul 25, 2012 at 11:00 PM, Martin Storsjö <mar...@martin.st> wrote:
>>>
>>> On Wed, 25 Jul 2012, Samuel Pitoiset wrote:
>>>
>>>> On Wed, Jul 25, 2012 at 8:05 PM, Martin Storsjö <mar...@martin.st>
>>>> 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.
Okay, I understand, thanks.
--
Best regards,
Samuel Pitoiset.
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
