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

[Frederic YHUEL]

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 ;-)


-- 
Frédéric