[vlc-devel] [PATCH 2/2] Add support for the new Android JellyBean MediaCodec API

[Martin Storsjö]

On Sun, 7 Oct 2012, Jean-Baptiste Kempf wrote:

> On Fri, Oct 05, 2012 at 09:45:42PM +0300, Martin Storsjö wrote :
>> +    int num_codecs, i;
>
> You know you can use C99, right? ;)

Right - I'll move the declarations to where they're actually used the 
first time.

>> +    switch (p_dec->fmt_in.i_codec) {
>> +    case VLC_CODEC_H264: mime = "video/avc"; break;
>> +    case VLC_CODEC_H263: mime = "video/3gpp"; break;
>> +    case VLC_CODEC_MP4V: mime = "video/mp4v-es"; break;
>> +    default:
>> +        msg_Err(p_dec, "codec %d not supported", p_dec->fmt_in.i_codec);
>
> Why a msg_Err, here?

Indeed, this probably shouldn't be printed at all, or at most as msg_Dbg.

>> +    msg_Dbg(p_dec, "%d codecs", num_codecs);
>
> Maybe a more explicit message here?

I guess this one could be dropped altogether, it's mostly interesting 
during early development of this module.

>> +        int num_types, j, found = 0;
>
> Shouldn't found be a bool ? OK, it does not matter ;)

Perhaps it should, I'll change it.

>> +    format = (*env)->CallStaticObjectMethod(env, p_sys->media_format_class, p_sys->create_video_format, (*env)->NewStringUTF(env, mime), p_dec->fmt_in.video.i_width, p_dec->fmt_in.video.i_height);
>
> Break the long line?

Good idea.

>
>> +		// TODO: Use crop_top/crop_left as well? Or is that already taken into account? Shouldn't be done on OMX_TI_COLOR_FormatYUV420PackedSemiPlanar at least.
>
> No tabs, please.

Whoops, sorry

>> +                p_pic->date = (*env)->GetLongField(env, p_sys->buffer_info, p_sys->pts_field);
>> +                GetVlcChromaSizes(p_dec->fmt_out.i_codec, p_dec->fmt_out.video.i_width, p_dec->fmt_out.video.i_height, NULL, NULL, &chroma_div);
>> +                CopyOmxPicture(p_sys->pixel_format, p_pic, p_sys->slice_height, p_sys->stride, ptr, chroma_div);
>
>
>> +        } else if (index == -3) { // INFO_OUTPUT_BUFFERS_CHANGED
>
> Should this be #define'd or in an enum?

I could do that, yes. Currently these are final static integers in the 
MediaCodec class, the java version of defines/enums for constants. But 
their value is fixed, and it's easier to just redeclare them as defines of 
our own than to write tedious JNI code to get their value.

>> +        memcpy(bufptr, p_block->p_buffer, size);
>
> <mode=av500> memcpy? there is a bug. :) </mode>

This API is pretty hard to use with direct rendering. Also, this 
particular memcpy is just for the input data, the one where the full 
picture is copied is hidden in the CopyOmxPicture function call. ;P

>> +        if (p_sys->nal_size) {
>> +            int len = size;
>> +            uint8_t *ptr = bufptr;
>> +            while (len >= p_sys->nal_size) {
>> +                uint32_t i, nal_len = 0;
>> +                for (i = 0; i < p_sys->nal_size; i++) {
>> +                    nal_len = (nal_len << 8) | ptr[i];
>> +                    ptr[i] = 0;
>> +                }
>> +                ptr[p_sys->nal_size - 1] = 1;
>> +                if (nal_len > INT_MAX || nal_len > (unsigned int) len)
>> +                    break;
>> +                ptr += nal_len + p_sys->nal_size;
>> +                len -= nal_len + p_sys->nal_size;
>> +            }
>> +        }
>
> Should this be splitted for reuse?

That might be useful, yes. I'll look into it.

> In general, not much to say :)

Ok, I'll return with an improved version soon. :-)

// Martin