Removing Camera Services Hardcoding

[steve2641]

Hello,

Within the startPreview() function of Camera Services there are
several hardcoded values that need to be addressed in order to
properly support different preview formats, buffer strides, and
potentially dynamic preview heap re-allocations. This last point
would be needed in cases such as image stabilization where the preview
heap may need to be re-sized on the fly.

I'm looking for feedback on a couple of possible approaches for this
support.

The first approach would be to simply extend what currently exists, as
in:

=================================
(CameraService.cpp, changes starting ~line 272)
// XXX: This needs to be improved. remove all hardcoded stuff

int w, h, ws, hs, frmt;
CameraParameters params(mHardware->getParameters());
params.getPreviewSize(&w, &h);
// New "preview-stride" = "<wstride>x<hstride>" parameter (new
params function)
// Would have to be set only by HAL, the app should not set it
params.getPreviewStride(&ws, &hs);
// Retrieve the preview format and convert to a surface format
(new function)
frmt = convertToSurfaceFormat( params.getPreviewFormat() );

mSurface->unregisterBuffers();

#if DEBUG_DUMP_PREVIEW_FRAME_TO_FILE
debug_frame_cnt = 0;
#endif

status_t ret = mHardware->startPreview(previewCallback,
mCameraService.get());
if (ret == NO_ERROR) {
mSurface->registerBuffers(w,h,ws,hs,frmt,
mHardware->getPreviewHeap());
}
else LOGE("mHardware->startPreview() failed with status %d\n",
ret);
=================================

This approach would add an odd camera parameter, in that only the HAL
should set it, but this would prevent any HAL API changes. Another
option would be to add a "mHardware->getPreviewStride()" function to
get the needed stride data, but this obviously would require a HAL API
change. Also, this approch does not address the possibility of the
preview heap being re-allocated, so it's only a partial solution.

If the HAL API is going to be modified, I'd propose a more complete
solution which adds a new HAL -> Camera Services callback to denote
when the preview heap has changed, as in:

=================================
(CameraHardwareInterface.h, add ~line 28)

/** Callback when the preview heap is updated */
typedef void (*pvheap_callback)(sp<IMemoryHeap> heap, int w, int h,
int wStride, int hStride, int format, void* user);

(CameraHardwareInterface.h, change ~line 91)
virtual status_t startPreview(preview_callback frameCb,
pvheap_callback heapCb, void* user) = 0;

(CameraService.cpp, changes starting ~line 272)
#if DEBUG_DUMP_PREVIEW_FRAME_TO_FILE
debug_frame_cnt = 0;
#endif

// The Preview Heap Callback will update the frame heap

status_t ret = mHardware->startPreview(previewCallback,
pvHeapCallback,
mCameraService.get());
if (ret != NO_ERROR)
LOGE("mHardware->startPreview() failed with status %d\n",
ret);

(CameraService.cpp, add)
// preview heap callback - called when an aspect of the preview heap
changes
void CameraService::Client::pvHeapCallback
(sp<IMemoryHeap> heap, int w, int h, int wStride, int hStride, int
format, void *user)
{
LOGV("pvChangeCallback");
sp<Client> client = getClientFromCookie(user);
if (client == 0) {
return;
}

Mutex::Autolock lock(client->mLock);

client->mSurface->unregisterBuffers();
client->mSurface->registerBuffers(w, h, wStride, hStride, format,
heap);
}
=================================

With this approach HAL would be required to call the pvheap_callback
function whenever the preview heap is allocated or re-allocated,
including the initial allocation. The pvHeap_callback would have to
be called prior to a frame using the new heap is provided via the
preview_callback function. I'm not sure if there are any impacts to
Surface Flinger here, I hope not. Also, as a cleanup the HAL
getPreviewHeap() function could potentially be removed.

Obviously I prefer the latter approach, since it solves all of the
issues I was attempting to cover, but the cost is a HAL API change.

Any comment or other views?

Steve.

[Dave Sparks]

Hi Steve,

I think your first approach is fine, i.e. having the camera return the
stride through a read-only key/value pair.

I'd like to understand more about dynamic changes in the preview heap.
Is it the case that the preview heap size may change while preview is
active? Also, is the preview heap chaning in response to a request by
the application, or is it something that can happen at any time due to
some internal state in the camera driver changing?

We are making a lot of changes in the area around the camera and video
sinks right now to support video overlay, which is leading to better
hardware abstraction. One area that we still don't have a good
solution for is surfacing device specific capabilities, particularly
where they cross driver boundaries. A good example of this is for
video recording, where the camera sensor has certain capabilities and
the video encoder has certain capabilities, and the actual device
capabilities are an intersection of the two.

[steve2641]

Dave,

Thanks for your quick reply.

Concerning the need for dynamic preview heap changes, we got the
impression from the sample Camera HAL implementation (specifically at
the top of the previewThread() function) that the application would be
allowed to change the preview size or frame rate or other preview
attributes while preview is running. If this is true, then doesn't
this imply the need for some sort of dynamic heap update feature?

In our specific case, we expect all changes to be driven from the
application. So, when the application changes the preview size or
enables/disables image stabilization or other attributes that affect
the preview heap then Camera Services would need to be told of these
changes.

If, by chance, the preview size and other attributes that effect the
heap size are not suppose to be changed while preview is running, how
is this (or how should this be) communicated to the application?

I think either case (dynamic support or not) we can produce the
affects that we are hoping for, but without dynamic support the
application has to do more work, stopping preview and such, which may
cause semi-perceivable delays on the final output.

Thanks,

Steve.

[Dave Sparks]

It doesn't seem difficult to deal with dynamic changes in preview. The
application will make a request through setParameters() and this may
result in a change in the preview heap.

It's too late to get this change into Cupcake, but we should be able
to work on this in the next release.

[steve2641]

Per Dave Sparks request, bug 1985 was entered to this issue.

On Jan 28, 2:02 pm, Dave Sparks <davidspa...@android.com> wrote:
> It doesn't seem difficult to deal with dynamic changes in preview. The
> application will make a request through setParameters() and this may
> result in a change in the preview heap.
>
> It's too late to get this change into Cupcake, but we should be able
> to work on this in the next release.
>
> On Jan 26, 11:16 am, steve2641 <steve2...@gmail.com> wrote:
>
>
>
> > Dave,
>
> > Thanks for your quick reply.
>
> > Concerning the need for dynamic preview heap changes, we got the

> > impression from the sampleCameraHAL implementation (specifically at

> > the top of the previewThread() function) that the application would be
> > allowed to change the preview size or frame rate or other preview
> > attributes while preview is running.  If this is true, then doesn't
> > this imply the need for some sort of dynamic heap update feature?
>
> > In our specific case, we expect all changes to be driven from the
> > application.  So, when the application changes the preview size or
> > enables/disables image stabilization or other attributes that affect

> > the preview heap thenCameraServices would need to be told of these

> > changes.
>
> > If, by chance, the preview size and other attributes that effect the
> > heap size are not suppose to be changed while preview is running, how
> > is this (or how should this be) communicated to the application?
>
> > I think either case (dynamic support or not) we can produce the
> > affects that we are hoping for, but without dynamic support the
> > application has to do more work, stopping preview and such, which may
> > cause semi-perceivable delays on the final output.
>
> > Thanks,
>

> > Steve.- Hide quoted text -
>
> - Show quoted text -