[Mesa-dev] [PATCH 00/20] MJPEG decode support through VA-API

[Leo Liu]

On 08/16/2017 03:22 AM, Christian König wrote:
> Hi Leo,
>
> Patches #2, #4, #6, #8 - #12, #14, #18-#20 are Reviewed-by: Christian 
> König <christian.koenig at amd.com>
>
> Patch #1:
>
> When you add new formats it would be nice to have 
> u_reduce_video_profile() in src/gallium/auxiliary/util/u_video.h 
> updated as well.

Yes. I have "u_reduce_video_profile()"  updated in patch 3.

The patch1 and patch2 are for vl headers, but I will move 
u_reduce_video_profile() to patch 1 in v2

>
> Additional to that I think the codec name is JPEG, not MJPEG. MJPEG is 
> just the file/stream format with multiple JPEGs.

Okay.

>
> Patch #3:
>
>> +        case PIPE_VIDEO_FORMAT_MJPEG:
>> +            return true;
> Please reorder the patch to be the last of the radeon/* patches, so 
> that the driver only starts to claim JPEG support when everything else 
> is implemented.

Sure in v2 patch 5, also add check for kernel version and ASICs.

>
> Apart from that the patch is Reviewed-by: Christian König 
> <christian.koenig at amd.com>
>
> Patch #5:
>> +    if (dec->stream_type != RUVD_CODEC_MJPEG) {
>> +        dpb_size = calc_dpb_size(dec);
>> +        if (!rvid_create_buffer(dec->screen, &dec->dpb, dpb_size, 
>> PIPE_USAGE_DEFAULT)) {
>> +            RVID_ERR("Can't allocated dpb.\n");
>> +            goto error;
>> +        }
>> +        rvid_clear_buffer(context, &dec->dpb);
> I think it would be cleaner if calc_dpb_size() just return zero for 
> RUVD_CODEC_MJPEG and then the calling code skipping buffer allocation 
> when it sees a zero sized dpb.

Sure in v2 patch3.

>
> Patch #7:
>> +      enum pipe_video_format format =
>> +         u_reduce_video_profile(context->templat.profile);
>> +
> Here you use u_reduce_video_profile, but I can't see where you update 
> u_reduce_video_profile() for JPEG support?

It's at original patch 3.

>
>
> Patch #13:
>
> Mhm, I'm not sure if it is a good idea to add this to the driver 
> backend, but the patch is Acked-by: Christian König 
> <christian.koenig at amd.com> anyway.
>
> Patch #15, #16 and #17 are somehow missing and didn't made it to the 
> mailing list.
 >The subject line should probably read "reallocate" instead of "relocate".

 >Additional to that the term "stacked field" isn't really used outside 
AMD, better use interlaced layout.

I will fix commit message.


 >      case RUVD_SURFACE_TYPE_LEGACY:
 >         msg->body.decode.dt_pitch = luma->u.legacy.level[0].nblk_x;
 >+        if (!chroma)
 >+            msg->body.decode.dt_pitch *= 2;

 >Patch looks good to me, except for this hunk.

 >Can we get the pitch somehow else instead of making it depending if 
chroma is present or not?

 >Cause that is clearly not correct in all cases.

I will add stream type check in v2.


Thanks for the review.

Leo


>
> Regards,
> Christian.
>
> Am 15.08.2017 um 22:08 schrieb Leo Liu:
>> The series is able to enable mjpeg decode support through vaapi, and 
>> that
>> includes for the formats of 420(NV12) and 422(YUYV).
>>
>> Leo Liu (20):
>>    vl: add mjpeg profile and format
>>    vl: add mjpeg picture description
>>    radeon/video: add mjpeg support
>>    radeon/uvd: add mjpeg stream type
>>    radeon/uvd: add mjpeg support
>>    st/va: add mjpeg picture to context
>>    st/va: create decoder for mjpeg format
>>    st/va: add handles for mjpeg Buffers
>>    st/va: add picture parameter handling for mjpeg
>>    st/va: add iq matrix handling for mjpeg
>>    st/va: add huffman table handling for mjpeg
>>    st/va: add slice parameter handling for mjpeg
>>    radeon/uvd: reconstruct mjpeg bitstream
>>    st/va: make surface allocate functions more usefully
>>    radeon/video: mjpeg not support stacked video buffers
>>    st/va: relocate surface when stack field false
>>    radeon/uvd: add yuyv format support for target buffer
>>    st/va: detect mjpeg format from bitstream
>>    st/va: relocate surface with yuyv stream
>>    st/va: add mjpeg for config
>>
>>   src/gallium/auxiliary/util/u_video.h           |   3 +
>>   src/gallium/drivers/radeon/radeon_uvd.c        | 175 
>> +++++++++++++++++++++++--
>>   src/gallium/drivers/radeon/radeon_uvd.h        |   1 +
>>   src/gallium/drivers/radeon/radeon_video.c      |   8 +-
>>   src/gallium/drivers/radeonsi/si_uvd.c          |   2 +-
>>   src/gallium/include/pipe/p_video_enums.h       |   6 +-
>>   src/gallium/include/pipe/p_video_state.h       |  59 +++++++++
>>   src/gallium/state_trackers/va/Makefile.sources |   1 +
>>   src/gallium/state_trackers/va/config.c         |   2 +-
>>   src/gallium/state_trackers/va/picture.c        |  70 +++++++++-
>>   src/gallium/state_trackers/va/picture_mjpeg.c  | 116 ++++++++++++++++
>>   src/gallium/state_trackers/va/surface.c        |   8 +-
>>   src/gallium/state_trackers/va/va_private.h     |  14 ++
>>   13 files changed, 440 insertions(+), 25 deletions(-)
>>   create mode 100644 src/gallium/state_trackers/va/picture_mjpeg.c
>>
>