>>>>> @@ -644,9 +646,9 @@ static int
>>>>> rtmp_handshake_imprint_with_digest(uint8_t
>>>>> *buf)
>>>>> int i, digest_pos = 0;
>>>>> int ret;
>>>>>
>>>>> - for (i = 8; i < 12; i++)
>>>>> + for (i = 772; i < 776; i++)
>>>>> digest_pos += buf[i];
>>>>> - digest_pos = (digest_pos % 728) + 12;
>>>>> + digest_pos = (digest_pos % 728) + 776;
>>>>>
>>>>
>>>> Why these changes? Aren't these codepaths already used by the normal rtmp
>>>> code? Or is it just plain wrong and not validated in practice there?
>>>> Anyway,
>>>> this feels like it should go into a separate commit if that's the case.
>>>
>>>
>>> These changes are needed for RTMPE, I saw that in librtmp... Anyway,
>>> it works fine with the normal rtmp.
>>>
>>> I'll separate it in a new patch.
>>
>>
>> "The changes are needed for RTMPE" is not a valid answer either, I'm sure
>> you need it because otherwise it wouldn't be in the patch. But what I want
>> to know is:
>>
>> 1) What does this do? This is needed for a few reasons: One is so that the
>> reviewer easily can understand it without having to read up on all details
>> on all the existing functions that the code calls. Another reason is that
>> you should explain what you intend the change to do. If someone later looks
>> at the commit for a reason, e.g. if the commit broke something or seems
>> buggy, one can see that the actual code isn't exactly what you explain, one
>> can easily find out that the bug was that the code didn't match what you
>> meant to do. If you don't explain it, one will never know whether the code
>> was intentionally done that way, or if it was a bug.
>>
>> 2) Why do you do this? ("Needed for RTMPE" is not a valid answer to that.
>> Something like "This data is ignored in normal RTMP but actually verified in
>> RTMPE, and due to that I realized that the existing code is wrong" or so.)
>>
>> 3) Since this is an existing codepath that you change, you also need to
>> explain what this change does to the existing use case, does it break
>> something existing, or motivate this apparently random change doesn't break
>> the existing use cases (plain RTMP).
>>
>> In principle every single patch (especially those that change existing code)
>> need to answer all of these questions in the commit message. If this is
>> missing, and a reviewer asks about it, you need to be able to produce
>> answers to these questions, otherwise one cannot easily accept the patch,
>> because one do not know what effect it has on existing code, thus I wouldn't
>> be sure it doesn't break anything.
>
> I still have to study this part in order to provide a *good* explanation.
Actually, that part was little wrong. Indeed, the digest position
computed in this function doesn't use the same values as in plain
rtmp.
I fixed that in the new diff.
--
Best regards,
Samuel Pitoiset.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
