Skip to content

Conversation

@cedricve
Copy link
Member

@cedricve cedricve commented Nov 11, 2025

Description

Pull Request: v3.3.21-onvif-0.0.7

Motivation and Improvement

This pull request aims to enhance the reliability and maintainability of the ONVIF integration within the cloud handling module. The key changes include:

  1. Commenting Out Unused Code:

    • Sections of the code related to the creation and handling of ONVIF pull point subscriptions have been commented out. This code was either not in use or causing issues. By commenting it out, we can prevent potential runtime errors and simplify debugging.
  2. Improved Error Handling:

    • Adjustments were made to the way devices connect to ONVIF. The code now handles errors more gracefully, ensuring that the system continues to operate even if the ONVIF device connection fails.
  3. Live Stream Handling Enhancements:

    • Introduced image resizing before converting images to bytes. This ensures that images are appropriately sized for further processing, potentially reducing bandwidth and storage requirements.
    • Added a feature to split images into chunks before sending them via MQTT. This prevents large messages from overwhelming the MQTT broker and ensures smoother data transmission.
  4. Code Refactoring:

    • Consolidated repetitive code and added meaningful comments to improve readability and maintainability.
    • Introduced a device ID variable for better identification and tracking of devices.
  5. Sleep Intervals to Prevent Flooding:

    • Added sleep intervals between sending MQTT messages to prevent flooding the MQTT broker, ensuring that the system remains stable under load.

These changes collectively enhance the stability, performance, and maintainability of the cloud handling module, especially in scenarios involving ONVIF devices and live streaming. By addressing potential error points and optimizing data handling, this update aims to provide a more robust and efficient system.

…ONVIF device connection handling and optimize image processing for MQTT streaming
Copilot AI review requested due to automatic review settings November 11, 2025 16:08
Copilot finished reviewing on behalf of cedricve November 11, 2025 16:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates ONVIF device connection handling by removing the unused second return value from ConnectToOnvifDevice calls and temporarily disables ONVIF event subscription functionality. It also optimizes image processing for MQTT streaming by adding image resizing before transmission and implementing an optional chunking mechanism for large images.

  • Updated ConnectToOnvifDevice function calls to match its actual signature (returns 2 values, not 3)
  • Added image resizing before MQTT transmission to reduce payload size
  • Implemented optional chunking for MQTT livestream to handle large images

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cameraConfiguration := configuration.Config.Capture.IPCamera
device, _, err := onvif.ConnectToOnvifDevice(&cameraConfiguration)
device, err := onvif.ConnectToOnvifDevice(&cameraConfiguration)
if err != nil {
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error check logic is inverted. If ConnectToOnvifDevice returns an error (err != nil), the code should not proceed to create a pull point subscription. This should be if err == nil { to match the pattern used elsewhere in the file (e.g., line 265).

Suggested change
if err != nil {
if err == nil {

Copilot uses AI. Check for mistakes.
cameraConfiguration := configuration.Config.Capture.IPCamera
device, _, err := onvif.ConnectToOnvifDevice(&cameraConfiguration)
device, err := onvif.ConnectToOnvifDevice(&cameraConfiguration)
if err != nil {
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error check logic is inverted. If ConnectToOnvifDevice returns an error (err != nil), the code should not proceed to unsubscribe. This should be if err == nil { to properly handle successful connections before cleanup.

Suggested change
if err != nil {
if err == nil {

Copilot uses AI. Check for mistakes.
log.Log.Info("cloud.HandleLiveStreamSD(): something went wrong while sending acknowledge config to hub: " + string(payload))

valueMap := make(map[string]interface{})
valueMap["image"] = bytes
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the non-chunking path, the image is sent as raw bytes without base64 encoding, which is inconsistent with the previous implementation that used base64 encoding (removed at line 712). This could break compatibility with receivers expecting base64-encoded data. Consider adding base64 encoding: valueMap[\"image\"] = base64.StdEncoding.EncodeToString(bytes).

Suggested change
valueMap["image"] = bytes
valueMap["image"] = base64.StdEncoding.EncodeToString(bytes)

Copilot uses AI. Check for mistakes.
Payload: models.Payload{
Version: "v1.0.0",
Action: "receive-sd-stream",
DeviceId: deviceId,
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent use of DeviceId field. In the chunking path, deviceId (local variable) is used, but in the non-chunking path (line 771) and HandleRealtimeProcessing (line 880), configuration.Config.Key is used directly. This inconsistency could lead to different device identifiers being sent depending on the chunking setting.

Copilot uses AI. Check for mistakes.
}
payload, err := models.PackageMQTTMessage(configuration, message)
if err == nil {
mqttClient.Publish("kerberos/hub/"+hubKey+"/"+deviceId, 1, false, payload)
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MQTT topic pattern is inconsistent between chunking and non-chunking modes. Chunking uses \"kerberos/hub/\"+hubKey+\"/\"+deviceId (line 757) while non-chunking uses \"kerberos/hub/\"+hubKey (line 777). This means chunks and non-chunked messages will be published to different topics, likely breaking message reception.

Copilot uses AI. Check for mistakes.
log.Log.Infof("cloud.HandleLiveStreamSD(): sent chunk %d/%d to MQTT topic kerberos/hub/%s/%s", i+1, len(chunks), hubKey, deviceId)
time.Sleep(33 * time.Millisecond) // Sleep to avoid flooding the MQTT broker with messages
} else {
log.Log.Info("cloud.HandleLiveStreamSD(): something went wrong while sending acknowledge config to hub: " + string(payload))
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message refers to 'acknowledge config' but this function is sending livestream chunks, not acknowledging config. The message should be more specific, e.g., 'error while packaging chunk message for MQTT' to accurately reflect the context.

Copilot uses AI. Check for mistakes.
if err == nil {
mqttClient.Publish("kerberos/hub/"+hubKey, 0, false, payload)
} else {
log.Log.Info("cloud.HandleLiveStreamSD(): something went wrong while sending acknowledge config to hub: " + string(payload))
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message refers to 'acknowledge config' but this function is sending livestream images, not acknowledging config. The message should be corrected to reflect the actual operation, e.g., 'error while packaging livestream message for MQTT'.

Copilot uses AI. Check for mistakes.
valueMap["chunkCount"] = len(chunks)
message := models.Message{
Payload: models.Payload{
Version: "v1.0.0",
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Version field is only set in the chunking path but not in the non-chunking path (line 769). This inconsistency in the message structure could cause issues for consumers expecting this field to always be present.

Copilot uses AI. Check for mistakes.
Comment on lines 721 to 724
// By default, bytes are not encoded to base64 here; you are splitting the raw JPEG/PNG bytes.
// However, in MQTT and web contexts, binary data may not be handled well, so base64 is often used.
// To avoid base64 encoding, just send the raw []byte chunks as you do here.
// If you want to avoid base64, make sure the receiver can handle binary payloads.
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment suggests sending raw bytes, but MQTT message payloads in Go are typically byte slices that get JSON-marshaled. Since valueMap is a map[string]interface{} that will be marshaled to JSON, the raw []byte in valueMap[\"chunk\"] will be base64-encoded automatically during JSON marshaling. The comment is misleading about avoiding base64 encoding.

Suggested change
// By default, bytes are not encoded to base64 here; you are splitting the raw JPEG/PNG bytes.
// However, in MQTT and web contexts, binary data may not be handled well, so base64 is often used.
// To avoid base64 encoding, just send the raw []byte chunks as you do here.
// If you want to avoid base64, make sure the receiver can handle binary payloads.
// Note: When you include []byte in a map[string]interface{} and marshal to JSON (as is done for MQTT payloads),
// the []byte will be base64-encoded automatically by Go's encoding/json package.
// Therefore, the receiver must be able to handle base64-encoded data in the "chunk" field.
// This is standard behavior for JSON in Go; you are not sending raw binary data here.

Copilot uses AI. Check for mistakes.
}
payload, err := models.PackageMQTTMessage(configuration, message)
if err == nil {
mqttClient.Publish("kerberos/hub/"+hubKey+"/"+deviceId, 1, false, payload)
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The QoS level differs between chunking (QoS 1 - at least once) and non-chunking (QoS 0 - at most once, line 777). For chunked messages where order and delivery matter, using QoS 1 is appropriate, but the non-chunking mode should also use QoS 1 for consistency, or this difference should be documented.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants