Re: [PATCH v2 5/6] [media] s5p-jpeg: Add support for resolution change event

[Thierry Escande <thierry.escande@collabora.com>]

Hi Andrzej,

On 20/06/2017 12:51, Andrzej Pietrasiewicz wrote:
> Hi Thierry,
> 
> W dniu 19.06.2017 o 15:50, Thierry Escande pisze:
>> Hi Andrzej,
>>
>> On 16/06/2017 17:38, Andrzej Pietrasiewicz wrote:
>>> Hi Thierry,
>>>
>>> Thank you for the patch.
>>>
>>> Can you give a use case for resolution change event?
>> Unfortunately, the original commit does not mention any clear use case.
>> I've asked to the patch author for more information.
> 
> Can you please share what you learn about it if the author gets back to 
> you?
> Now that we don't know why to apply a patch I guess we should not do it.
This event is used in Chromium by the V4L2 jpeg decode accelerator to 
allocate output buffer. Please see:
https://cs.chromium.org/chromium/src/media/gpu/v4l2_jpeg_decode_accelerator.cc?rcl=91793c6ef94f05e93d258db8c7f3cad59819c6b8&l=585

I'll add a note in the commit message.

> 
> <snip>
> 
>>>> @@ -2510,43 +2567,18 @@ static void s5p_jpeg_buf_queue(struct
>>>> vb2_buffer *vb)
>>>>                return;
>>>>            }
>>>> -        q_data = &ctx->out_q;
>>>> -        q_data->w = tmp.w;
>>>> -        q_data->h = tmp.h;
>>>> -        q_data->sos = tmp.sos;
>>>> -        memcpy(q_data->dht.marker, tmp.dht.marker,
>>>> -               sizeof(tmp.dht.marker));
>>>> -        memcpy(q_data->dht.len, tmp.dht.len, sizeof(tmp.dht.len));
>>>> -        q_data->dht.n = tmp.dht.n;
>>>> -        memcpy(q_data->dqt.marker, tmp.dqt.marker,
>>>> -               sizeof(tmp.dqt.marker));
>>>> -        memcpy(q_data->dqt.len, tmp.dqt.len, sizeof(tmp.dqt.len));
>>>> -        q_data->dqt.n = tmp.dqt.n;
>>>> -        q_data->sof = tmp.sof;
>>>> -        q_data->sof_len = tmp.sof_len;
>>>> -
>>>> -        q_data = &ctx->cap_q;
>>>> -        q_data->w = tmp.w;
>>>> -        q_data->h = tmp.h;
>>>
>>>
>>> Why is this part removed?
>> This has not been removed.
>> The &tmp s5p_jpeg_q_data struct was passed to s5p_jpeg_parse_hdr() and
>> then copied field-by-field into ctx->out_q (through q_data pointer).
>> With this change ctx->out_q is passed to s5p_jpeg_parse_hdr() and this
>> avoids the copy.
> 
> It seems that changing field-by-field copying to passing a pointer
> directly to s5p_jpeg_parse_hdr() is an unrelated change and as such
> should be in a separate patch.
Will do.

Regards,
  Thierry