[vlc-devel] [PATCH 1/2] httplive: fix Read func. (when caller skips data)

[Jean-Paul Saman]

On Mon, Feb 6, 2012 at 2:11 PM, Frederic YHUEL <fyhuel at viotech.net> wrote:
> On Fri, Feb 3, 2012 at 9:44 AM, Frederic YHUEL <fyhuel at viotech.net> wrote:
>> On Wed, Feb 1, 2012 at 2:52 PM, Hugo Beauzée-Luyssen <beauze.h at gmail.com> wrote:
>>> On Wed, Feb 1, 2012 at 1:13 PM, Frederic YHUEL <fyhuel at viotech.net> wrote:
>>>> On Wed, Feb 1, 2012 at 12:28 PM, Hugo Beauzée-Luyssen
>>>> <beauze.h at gmail.com> wrote:
>>>>> On Wed, Feb 1, 2012 at 12:13 PM, Jean-Baptiste Kempf <jb at videolan.org> wrote:
>>>>>> On Tue, Jan 31, 2012 at 01:32:22PM +0100, Frederic YHUEL wrote :
>>>>>>> -            memcpy(p_read + copied, segment->data->p_buffer, len);
>>>>>>> +            if( p_read ) /* otherwise caller skips data */
>>>>>>> +                memcpy(p_read + copied, segment->data->p_buffer, len);
>>>>>>
>>>>>> The code seems wrong if hls_read can be called with p_read at NULL.
>>>>>> Use assert then.
>>>>>>
>>>>>>> -    if (buffer == NULL)
>>>>>>> -    {
>>>>>>> -        /* caller skips data, get big enough buffer */
>>>>>>> -        msg_Warn(s, "buffer is NULL (allocate %d)", i_read);
>>>>>>> -        buffer = calloc(1, i_read);
>>>>>>> -        if (buffer == NULL)
>>>>>>> -            return 0; /* NO MEMORY left*/
>>>>>>> -    }
>>>>>>
>>>>>> What guarrantees us that the buffer will not be NULL ?
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>
>>>>> If I'm not mistaking, the all point is to allow buffer to be NULL.
>>>>> This patch looks correct to me, I don't see the point in allocating
>>>>> memory and copying data to it if the data is to be skipped.
>>>>
>>>> +1
>>>>
>>>>> Maybe adding a comment stating that p_read may be NULL at the
>>>>> beginning of the hls_Read function would be better, so one doesn't
>>>>> assume p_read is always != NULL
>>>>>
>>>>
>>>> Indeed, it would probably have helped my point :-)
>>>> Here is a new patch with such a comment.
>>>>
>>>> Thanks,
>>>>
>>>
>>> Works for me.
>>>
>>
>> Oh I forgot to say that this patch has been tested thoroughly in HLS,
>> and also in my Smooth Streaming module, which has virtually the same
>> function, and which implements a forward seek by a call to sms_Read(
>> s, NULL, len ) (which is not very smart but that's another problem).
>>
>> If the fact of removing this useless buffer allocation / memcpy were
>> bad, I should have notice by the time.
>>
>
> Hello,
>
> Could someone apply the patch, or explain why Hugo and I are wrong?
>
> j-b, jpsaman?
>
> That's my last attempt, after that I'll stop whining, I swear ;-)

Patch Applied, thanks for being persistent ;)


Kind regards,

Jean-Paul Saman.