[FFmpeg-devel] [PATCH] Add android_capture indev

[Felix Matouschek]

Hello,

Am 02.11.2017 um 14:40 schrieb Nicolas George:
> When reading the subject of the mail, I first thought it would be about
> screen capture. Furthermore, there is code for audio, but it is not
> connected to anything, and it does not seem that the android API
> connects audio and video together.
>
> For all these reasons, I suggest you name this device maybe
> android_camera, and keep android_mic for audio capture.
Will do so.

> You are parsing the "filename" of the device into a key=value syntax.
> This is not a good idea, and I really would not like another key=value
> parser in the code base.
>
> How do cameraIds look? If the string values are friendly enough, they
> can be used as filenames as is.
I removed parsing of the "filename" and replaced it with a 
"camera_index" parameter.

I do not know how cameraIds are formed on different devices, seems like 
internal cameras
just use numbers but external devices may use a string. I do not have 
multiple devices to test
this. In my opionion an index for the list of all cameras should be 
sufficient.

> Can the error be translated into a human-readable message?
Will try to do that, I need to add the according strings for it somehow.

> Is it really the official way of getting the resolution and format of
> the video? If so, were the Android people drunk?
Seem like it.

>> +        if (format == IMAGE_FORMAT_ANDROID) {
> It would be better to support as many pixel formats as possible.
For now I settled on YUV420P, as the API doc states all devices must 
support it. I do not
have the resources to test every format.

>> +    side_data = av_packet_new_side_data(&pktl_next->pkt,
>> +            AV_PKT_DATA_DISPLAYMATRIX, sizeof(display_matrix));
> It would probably be best to avoid sending the side data repeatedly if
> it does not change.
Is it sufficient to append the matrix just once? I thought every 
AVPacket could have a different matrix.

>> +error:
>> +    pthread_mutex_unlock(&ctx->mutex);
>> +    AImage_delete(image);
>> +
>> +    return;
> It seems error conditions are not taken into account. Is it on purpose?
You mean for example ENOMEM if allocating fails and aborting the whole 
"session"? Could do that.

> Regards, 

I working on addressing all issues.

Greetings,
Felix