📝 add dual stream support for 3G-B interface in VideoMaster#13
📝 add dual stream support for 3G-B interface in VideoMaster#13
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a dual_stream option to the VideoMaster input device to support forcing the 3G-B Level B Dual-Stream (B-DS) SDI interface when auto-detection is not possible (closes #12).
Changes:
- Introduces
dual_streamas an AVOption, threads it through parsing/context, and documents it. - Extends the video stream property query API to accept a
dual_streamflag and adds a capability check for 3G-B DS support. - Updates SDI property probing to support the forced 3G-B DS interface path.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| libavdevice/videomaster_dec.c | Adds dual_stream option, validation, and passes flag through initialization/property checks. |
| libavdevice/videomaster_common.h | Adds dual_stream to context/data and extends the video properties function signature; declares DS capability helper. |
| libavdevice/videomaster_common.c | Implements DS capability helper, adds SDI dual-stream probing logic, and introduces RX clock-divisor property mapping helper. |
| doc/indevs.texi | Documents the new dual_stream option and provides an example usage. |
| .gitignore | Ignores .vscode/ workspace folder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { "dual_stream", | ||
| "Force 3GB dual stream interface (that cannot be auto-detect). A 3G " | ||
| "Level B-DS stream received on the RX0 physicla connector is received by " | ||
| "two independant streams: one RX0 stream received the A link and one RX1 " |
There was a problem hiding this comment.
Spelling: “independant” should be “independent”.
| "two independant streams: one RX0 stream received the A link and one RX1 " | |
| "two independent streams: one RX0 stream received the A link and one RX1 " |
| * @return uint32_t The RX SDI board property | ||
|
|
||
| */ | ||
| static uint32_t |
There was a problem hiding this comment.
The forward declaration marks get_rx_sdi_board_property_clock_divisor_from_index() as static, but the definition later is non-static. This causes a linkage mismatch and can fail to compile with conflicting storage-class diagnostics. Make the definition static (or remove static from the prototype) so they match.
| static uint32_t | |
| uint32_t |
| case 11: | ||
| return VHD_SDI_BP_RX11_CLOCK_DIV; | ||
| default: | ||
| return VHD_SDI_BP_RX0_CLOCK_DIV; |
There was a problem hiding this comment.
get_rx_sdi_board_property_clock_divisor_from_index() returns RX0’s property for any unknown index. That can silently use the wrong clock divisor when channel_index is out of the supported range (and it’s inconsistent with get_rx_stream_type_from_index(), which returns an invalid sentinel). Consider returning an invalid/"NB" value and having the caller detect/report EINVAL instead of falling back to RX0.
| return VHD_SDI_BP_RX0_CLOCK_DIV; | |
| /* Invalid index: return sentinel value instead of falling back to RX0. */ | |
| return 0; |
| HANDLE stream_handle = NULL; | ||
| VHD_OpenStreamHandle(board_handle, | ||
| get_rx_stream_type_from_index(channel_index), | ||
| VHD_SDI_STPROC_JOINED, NULL, &stream_handle, | ||
| NULL); | ||
| VHD_SetStreamProperty(stream_handle, VHD_SDI_SP_INTERFACE, | ||
| video_info->sdi.interface); | ||
|
|
||
| handle_vhd_status(avctx, | ||
| VHD_GetStreamProperty( | ||
| stream_handle, VHD_SDI_SP_VIDEO_STANDARD, | ||
| (uint32_t *)&video_info->sdi.video_standard), | ||
| "", ""); | ||
| int status = VHD_StartStream(stream_handle); | ||
|
|
||
| handle_vhd_status(avctx, | ||
| VHD_GetStreamProperty( | ||
| stream_handle, VHD_SDI_SP_VIDEO_STANDARD, | ||
| (uint32_t *)&video_info->sdi.video_standard), | ||
| "", ""); | ||
|
|
||
| handle_vhd_status( | ||
| avctx, | ||
| VHD_GetBoardProperty( | ||
| board_handle, | ||
| get_rx_sdi_board_property_clock_divisor_from_index( | ||
| channel_index), | ||
| (uint32_t *)&video_info->sdi.clock_divisor), | ||
| "", ""); | ||
| if (status == VHDERR_NOERROR) | ||
| VHD_StopStream(stream_handle); | ||
| VHD_CloseStreamHandle(stream_handle); |
There was a problem hiding this comment.
In the dual_stream SDI path, a new local variable named stream_handle shadows the function parameter, and VHD_OpenStreamHandle/VHD_SetStreamProperty/VHD_StartStream return values are not checked before using/closing the handle. If opening the handle fails, subsequent calls will operate on NULL and may crash. Use a differently named local handle (or reuse local_stream_handle), wrap VHD_* calls with handle_vhd_status (and honor its return), and only stop/close when the handle was successfully opened.
| HANDLE stream_handle = NULL; | |
| VHD_OpenStreamHandle(board_handle, | |
| get_rx_stream_type_from_index(channel_index), | |
| VHD_SDI_STPROC_JOINED, NULL, &stream_handle, | |
| NULL); | |
| VHD_SetStreamProperty(stream_handle, VHD_SDI_SP_INTERFACE, | |
| video_info->sdi.interface); | |
| handle_vhd_status(avctx, | |
| VHD_GetStreamProperty( | |
| stream_handle, VHD_SDI_SP_VIDEO_STANDARD, | |
| (uint32_t *)&video_info->sdi.video_standard), | |
| "", ""); | |
| int status = VHD_StartStream(stream_handle); | |
| handle_vhd_status(avctx, | |
| VHD_GetStreamProperty( | |
| stream_handle, VHD_SDI_SP_VIDEO_STANDARD, | |
| (uint32_t *)&video_info->sdi.video_standard), | |
| "", ""); | |
| handle_vhd_status( | |
| avctx, | |
| VHD_GetBoardProperty( | |
| board_handle, | |
| get_rx_sdi_board_property_clock_divisor_from_index( | |
| channel_index), | |
| (uint32_t *)&video_info->sdi.clock_divisor), | |
| "", ""); | |
| if (status == VHDERR_NOERROR) | |
| VHD_StopStream(stream_handle); | |
| VHD_CloseStreamHandle(stream_handle); | |
| HANDLE local_stream_handle = NULL; | |
| int open_status = handle_vhd_status( | |
| avctx, | |
| VHD_OpenStreamHandle( | |
| board_handle, | |
| get_rx_stream_type_from_index(channel_index), | |
| VHD_SDI_STPROC_JOINED, NULL, &local_stream_handle, NULL), | |
| "", ""); | |
| if (open_status == VHDERR_NOERROR && local_stream_handle) | |
| { | |
| handle_vhd_status( | |
| avctx, | |
| VHD_SetStreamProperty(local_stream_handle, | |
| VHD_SDI_SP_INTERFACE, | |
| video_info->sdi.interface), | |
| "", ""); | |
| handle_vhd_status( | |
| avctx, | |
| VHD_GetStreamProperty( | |
| local_stream_handle, VHD_SDI_SP_VIDEO_STANDARD, | |
| (uint32_t *)&video_info->sdi.video_standard), | |
| "", ""); | |
| int start_status = handle_vhd_status( | |
| avctx, VHD_StartStream(local_stream_handle), "", ""); | |
| handle_vhd_status( | |
| avctx, | |
| VHD_GetStreamProperty( | |
| local_stream_handle, VHD_SDI_SP_VIDEO_STANDARD, | |
| (uint32_t *)&video_info->sdi.video_standard), | |
| "", ""); | |
| handle_vhd_status( | |
| avctx, | |
| VHD_GetBoardProperty( | |
| board_handle, | |
| get_rx_sdi_board_property_clock_divisor_from_index( | |
| channel_index), | |
| (uint32_t *)&video_info->sdi.clock_divisor), | |
| "", ""); | |
| if (start_status == VHDERR_NOERROR) | |
| VHD_StopStream(local_stream_handle); | |
| VHD_CloseStreamHandle(local_stream_handle); | |
| } |
| if (check_dual_stream(videomaster_context) != 0) | ||
| { | ||
| av_log(videomaster_context->avctx, AV_LOG_ERROR, | ||
| "Failed to check dual-stream integrity\n"); | ||
| ff_videomaster_close_stream_handle(videomaster_context); | ||
| ff_videomaster_close_board_handle(videomaster_context); | ||
| return AVERROR(EIO); | ||
| } |
There was a problem hiding this comment.
check_header_arguments() calls check_dual_stream() before check_channel_index(). check_dual_stream() queries capabilities using videomaster_context->channel_index, so an out-of-range channel_index will produce misleading errors. Also, on failure this block always returns AVERROR(EIO), discarding check_dual_stream()’s AVERROR(EINVAL), and it calls ff_videomaster_close_stream_handle() even though the stream handle is not opened yet (can trigger spurious close errors). Validate channel_index first (or add range/channel-type validation inside check_dual_stream), propagate the original error code, and only close the stream handle when it is non-NULL.
| "Level B-DS stream received on the RX0 physicla connector is received by " | ||
| "two independant streams: one RX0 stream received the A link and one RX1 " |
There was a problem hiding this comment.
Spelling: “physicla” should be “physical”.
| "Level B-DS stream received on the RX0 physicla connector is received by " | |
| "two independant streams: one RX0 stream received the A link and one RX1 " | |
| "Level B-DS stream received on the RX0 physical connector is received by " | |
| "two independent streams: one RX0 stream received the A link and one RX1 " |
close #12