Add Gtest support to test Camera-vhal API's #11

Sapna1-singh · 2023-02-16T05:35:12Z

Introduce a property based on which instance id is taken for socket connection for Gtest purpose. Check for this property in camera-vhal and if this set then use the instance id that is set by property for Gtest else use the default one.

Newly introduced property is ro.boot.container.testid.

Add Unit TestCases for Camera-vhal in Gtest.

Tracked-On: OAM-105831
Signed-off-by: Singh, Sapna1 [email protected]

akodanka · 2023-02-21T05:00:58Z

Gtest/Android.mk

+LOCAL_CFLAGS += -fexceptions
+LOCAL_MODULE_TAGS:= optional
+
+LOCAL_SRC_FILES := main.cpp CameraFixture.cpp streamer_simulation_camera_client.cpp


Can you use the same naming conventions for the files , Classes and functions ?

akodanka · 2023-02-21T05:01:46Z

Gtest/Android.mk

+LOCAL_MODULE_TAGS:= optional
+
+LOCAL_SRC_FILES := main.cpp CameraFixture.cpp streamer_simulation_camera_client.cpp
+LOCAL_SHARED_LIBRARIES += liblog libcutils libutils camera.cic_cloud libvhal-client


I guess it would be better to use camera.$(TARGET_PRODUCT) instead of hardcoding.

akodanka · 2023-02-21T05:26:30Z

Gtest/CameraFixture.cpp

+
+
+void CameraFixture::runStreamer(){
+    cc.IsRunning = true;


why is IsRunning a static variable and not a member ?
Also, I think initializing it to true can be done from startDummyStreamer function internally.
Similarly setting to false can be done through a simple stopStreamer API. Thus it can be a private member too.

akodanka · 2023-02-21T05:33:03Z

Gtest/CameraFixture.cpp

+CameraFixture::CameraFixture()
+{
+    SetCallback();
+    t1 = new thread(&CameraFixture::runStreamer, this);


Memory Leak ? This object t1 is not destructed.

akodanka · 2023-02-21T05:36:59Z

Gtest/CameraFixture.h

+#include "VirtualCameraFactory.h"
+#include "streamer_simulation_camera_client.h"
+
+#define clientId 0


This is a very common variable, and shouldn't be defined as a macro.
And, I think its better to use upper case for all macros.

akodanka · 2023-02-21T06:47:31Z

Gtest/streamer_simulation_camera_client.cpp

+          [&](const VideoSink::camera_config_cmd_t& ctrl_msg) {
+            switch (ctrl_msg.cmd) {
+                case VideoSink::camera_cmd_t::CMD_OPEN: {
+                    cout << "Received Open command from Camera VHal\n";


Printing a TAG and func might be helpful for debugging.

akodanka · 2023-02-21T06:48:35Z

include/VirtualCameraFactory.h

+    /*
+    * Returns the shared pointer for client Communicator
+    */
+    std::vector<std::shared_ptr<ClientCommunicator>> getClientThread() { return mClientThreads; }


Why not pass the clientId and just return the particular ClientThread ?

akodanka · 2023-02-21T07:15:47Z

src/ClientCommunicator.cpp

+        val_client_cap[i].validOrientation = IsSensorOrientationValid(camera_info[i].sensorOrientation);
+        val_client_cap[i].validCameraFacing = IsCameraFacingValid(camera_info[i].facing);
+
+        /*switch (camera_info[i].codec_type) {


You can delete these lines instead of commenting out.

akodanka · 2023-02-21T07:18:46Z

src/ClientCommunicator.cpp

@@ -141,6 +172,8 @@ void ClientCommunicator::configureCapabilities() {
    if (header.type != CAMERA_INFO) {
        ALOGE("%s(%d): invalid camera_packet_type: %s", __FUNCTION__, mClientId,
              camera_type_to_str(header.type));
+        if(header.type == REQUEST_CAPABILITY)


Since this is not a valid usecase, I wouldn't recommend using a goto statement here.
If we really want to support the REQUEST_CAPABILITY dynamically, we should modularize all of these socket communication code, to save the current state, and handle the state changes accordingly.

src/VirtualCameraFactory.cpp

akodanka · 2023-03-02T09:14:06Z

include/ClientCommunicatorHelper.h

+#ifndef CLIENT_COMMUNICATOR_HELPER_H
+#define CLIENT_COMMUNICATOR_HELPER_H
+
+#include <iostream>


Do we really need these 2 headers included ?

Its better to keep these 2 headers to avoid redefinition of header file issue.

akodanka · 2023-03-02T09:15:51Z

include/ClientCommunicatorHelper.h

+
+namespace android {
+
+class ClientCommunicatorHelper {


Unless we have some ClientCommunication specific contents to be added in this file, I think it would be better to rename this something like CapabilitiesHelper ?

akodanka · 2023-03-02T09:16:36Z

src/ClientCommunicatorHelper.cpp

+#include <ClientCommunicatorHelper.h>
+#include "CameraSocketCommand.h"
+#include <log/log.h>
+#include <sys/socket.h>


Again please check if all these headers are required.

akodanka · 2023-03-03T04:59:13Z

src/ClientCommunicator.cpp

@@ -297,22 +233,24 @@ void ClientCommunicator::configureCapabilities() {
        // Wait till complete the metadata update for a camera.
        {
            Mutex::Autolock al(sMutex);
-            // Dynamic client configuration is not supported when camera


Dynamic configuration check is not required anymore ?

No its not required. Its already fixed from AIC application code.

akodanka · 2023-03-03T05:01:00Z

src/ClientCommunicator.cpp

@@ -321,16 +259,14 @@ void ClientCommunicator::configureCapabilities() {
    if (send(mClientFd, ack_packet, ack_packet_size, 0) < 0) {
        ALOGE("%s(%d): Failed to send camera capabilities, err: %s ", __FUNCTION__, mClientId,
              strerror(errno));
-        goto out;
+        free(ack_packet);


I think you can avoid multiple free calls if you put the below logs in else block, and remove this line ?

akodanka · 2023-03-03T05:08:36Z

src/ClientCommunicator.cpp

+                            break;
+                        case CAMERA_DATA:
+                            if (!mIsConfigurationDone) {
+                                ALOGE("%s(%d): Not Valid Type.....camera_packet_type: %s, line_num = %d ", __FUNCTION__, mClientId,


Can you mention Configuration Incomplete in the logs to indicate why the packet is invalid.
eg.

ALOGE("%s(%d): Invalid camera_packet_type: %s, Configuration not completed", __FUNCTION__, mClientId, camera_type_to_str(header.type));```

akodanka · 2023-03-03T05:11:44Z

src/ClientCommunicator.cpp

+                                handleIncomingFrames(header.size);
+                            break;
+                        default:
+                            if (header.size > mSocketBuffer.size()) {


I don't think this condition should be part of the default case, instead it can be checked before entering the switch case probably.
Even if its targeted only for data packets, I think it should be handled at the beginning of case CAMERA_DATA: .

akodanka · 2023-03-03T05:42:29Z

Gtest/CameraClient.h

+
+class CameraClient {
+private:
+    bool IsRunning;


I think we should have a uniform naming convention for the variables(at least for the newly introduced source files).
Could be mIsRunning, is_running_, but should be same across the variables used (instance_id , video_sink etc).

Modularize all the socket communication code for handling all the incoming requests dynamically and add a helper class that facilitates validation of all the camera info metadata. Tracked-On: OAM-106780 Signed-off-by: Singh, Sapna1 <[email protected]>

Introduce a property based on which instance id is taken for socket connection for Gtest purpose. Check for this property in camera-vhal and if this set then use the instance id that is set by property for Gtest else use the default one. Newly introduced property is ro.boot.container.testid. Add Unit TestCases for Camera-vhal in Gtest. Tracked-On: OAM-105831 Signed-off-by: Singh, Sapna1 [email protected]

ConnectionsListener class was derived from Thread class which was causing issue while calling join for this thread from VirtualCameraFactory destructor. The Base class from which ConnectionsListener was derived was not the proper class. With current implementation, instead of deriving from thread class, a thread is introduced as a member of the class to do the same job and it is started during creation of ConnectionsListener object Tracked-On: OAM-106781 Signed-off-by: Singh, Sapna1 <[email protected]>

close the client thread after completing the execution of all testcases. An API is introduced to do cleanup and in that API, client thread exit is requested, socket server thread exit is requested and joins with the main thread after all the cleanup. Tracked-On: OAM-106783 Signed-off-by: Singh, Sapna1 <[email protected]>

Sapna1-singh force-pushed the ww07_Gtest branch from 2ff2b56 to 51be974 Compare February 16, 2023 05:36

akodanka suggested changes Feb 21, 2023

View reviewed changes

Sapna1-singh force-pushed the ww07_Gtest branch 6 times, most recently from 6bcebd7 to 7267fde Compare February 23, 2023 09:30

akodanka reviewed Mar 3, 2023

View reviewed changes

Sapna1-singh added 3 commits March 10, 2023 16:35

Sapna1-singh force-pushed the ww07_Gtest branch from 7267fde to 3ddd23c Compare March 10, 2023 11:22

Sapna1-singh force-pushed the ww07_Gtest branch from 3ddd23c to c670149 Compare March 10, 2023 11:41

Sapna1-singh added 2 commits March 10, 2023 17:19

Merge branch 'r/ics3a/main' into ww07_Gtest

4406922

Update ClientCommunicator.cpp

56be12d

sysopenci added the Stale Stale label for inactive open prs label Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Gtest support to test Camera-vhal API's #11

Add Gtest support to test Camera-vhal API's #11

Sapna1-singh commented Feb 16, 2023 •

edited

Loading

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Feb 21, 2023

akodanka Mar 2, 2023

Sapna1-singh Mar 10, 2023

akodanka Mar 2, 2023

akodanka Mar 2, 2023

akodanka Mar 3, 2023

Sapna1-singh Mar 10, 2023

akodanka Mar 3, 2023

akodanka Mar 3, 2023

akodanka Mar 3, 2023

akodanka Mar 3, 2023

Add Gtest support to test Camera-vhal API's #11

Are you sure you want to change the base?

Add Gtest support to test Camera-vhal API's #11

Conversation

Sapna1-singh commented Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sapna1-singh commented Feb 16, 2023 •

edited

Loading