[vlc-devel] [PATCH]: flaschen out: Fix offset TODO and allow for larger displays

[Marvin Scholz]

Hi, thanks for the patch.
I just had a quick look, some remark below:

> commit 0aaf6c1574da6914eecfa29deae8b203b92bf281
> Author: Henner Zeller <h.zeller at acm.org>
> Date:   Sat Mar 21 13:47:53 2020 -0700
>
>     FlaschenTaschen output: Fix offset TODO and allow for larger 
> displays.
>         * Implement a long-standing TODO to use the FlaschenTaschen 
> feature to
>        choose the location as well as the layer when sending to the
>        screen.
>         * In the course of that: split the output into horizontal
>        tiles when it can't be send in a single UDP packet.
>        tested:
>        * make check passes
>        * tested on a LED screen with 192x128 pixel which requires
>          two UDP packets to update.
>        Signed-off-by: Henner Zeller <h.zeller at acm.org>
>
> diff --git a/modules/video_output/flaschen.c 
> b/modules/video_output/flaschen.c
> index 00e1e66a34..d7f872c792 100644
> --- a/modules/video_output/flaschen.c
> +++ b/modules/video_output/flaschen.c
> @@ -43,6 +43,13 @@
>  #include <vlc_plugin.h>
>  #include <vlc_vout_display.h>
> +struct flaschen_offset_t {
> +    int x;
> +    int y;
> +    int z;  /* aka. layer */
> +};
> +static struct flaschen_offset_t flaschen_offset_from_flag;

With this being global, wouldn’t it cause issues with multiple
vouts at the same time?

> +
>  #define T_FLDISPLAY N_("Flaschen-Taschen display address")
>  #define LT_FLDISPLAY N_( \
>      "IP address or hostname of the Flaschen-Taschen display. " \
> @@ -54,6 +61,10 @@
>  #define T_HEIGHT N_("Height")
>  #define LT_HEIGHT N_("Video height")
> +#define T_OFFSET N_("Offset")
> +#define LT_OFFSET N_("Comma separated x,y,z offset. Z also known as 
> layer. " \
> +                     "Just providing only x or x,y allowed.")
> +
>  static int Open(vout_display_t *vd, const vout_display_cfg_t *cfg,
>                  video_format_t *fmtp, vlc_video_context *context);
>  static void Close(vout_display_t *vd);
> @@ -69,6 +80,7 @@ vlc_module_begin ()
>      add_string( "flaschen-display", NULL, T_FLDISPLAY, LT_FLDISPLAY, 
> true )
>      add_integer("flaschen-width", 25, T_WIDTH, LT_WIDTH, false)
>      add_integer("flaschen-height", 20, T_HEIGHT, LT_HEIGHT, false)
> +    add_string( "flaschen-offset", "0,0,0", T_OFFSET, LT_OFFSET, 
> false )

No strong preference here but wouldn’t it be easier to just have 
flaschen-offset-x
flaschen-offset-y, and flaschen-offset-z?

>  vlc_module_end ()
>  @@ -110,6 +122,21 @@ static int Open(vout_display_t *vd, const 
> vout_display_cfg_t *cfg,
>      /* p_vd->info is not modified */
> +    memset(&flaschen_offset_from_flag, 0, 
> sizeof(flaschen_offset_from_flag));
> +    char *offset = var_InheritString(vd, "flaschen-offset");
> +    if (offset)
> +    {
> +        if (sscanf(offset, "%d,%d,%d",
> +                   &flaschen_offset_from_flag.x,
> +                   &flaschen_offset_from_flag.y,
> +                   &flaschen_offset_from_flag.z) < 1)
> +         {
> +            msg_Warn(vd, "At least x value required for 
> flaschen-offset.");
> +            /* non critical, continue */
> +         }
> +         free(offset);
> +    }
> +
>      char *display = var_InheritString(vd, "flaschen-display");
>      if (display == NULL) {
>          msg_Err(vd, "missing flaschen-display");
> @@ -162,48 +189,67 @@ static void Display(vout_display_t *vd, 
> picture_t *picture)
>  #else
>      const long iovmax = sysconf(_SC_IOV_MAX);
>  #endif
> -    vout_display_sys_t *sys = vd->sys;
> -    video_format_t *fmt = &picture->format;
> -    int result;
> -
> -    char buffer[64];
> -    int header_len = snprintf(buffer, sizeof(buffer), "P6\n%d 
> %d\n255\n",
> -                              fmt->i_width, fmt->i_height);
> -    /* TODO: support offset_{x,y,z}? (#FT:...) */
> -    /* Note the protocol doesn't include any picture order field. */
> -    /* (maybe add as comment?) */
> -
> -    int iovcnt = 1 + fmt->i_height;
> -    if (unlikely(iovcnt > iovmax))
> -        return;
> -
> -    struct iovec iov[iovcnt];
> -    iov[0].iov_base = buffer;
> -    iov[0].iov_len = header_len;
> -
> -    uint8_t *src = picture->p->p_pixels;
> -    for (int i = 1; i < iovcnt; i++)
> +    const vout_display_sys_t *sys = vd->sys;
> +    const video_format_t *fmt = &picture->format;
> +
> +    /* Local offset copy needed to modify when tiling large frames */
> +    struct flaschen_offset_t offset = flaschen_offset_from_flag;
> +
> +    /* For a huge display, the full frame might not fit in one UDP 
> packet.
> +     * So we send it in horizontal strips that each fit.
> +     */
> +    const int kMaxDataLen = 65507 - 64;  /* Leave some space for 
> header */
> +    const size_t row_size = fmt->i_width * 3;
> +    const int max_send_height = kMaxDataLen / row_size;
> +
> +    char header_buffer[64];
> +    uint8_t *picture_buffer = picture->p->p_pixels;
> +    int rows = fmt->i_height;
> +    while (rows)
>      {
> -        iov[i].iov_base = src;
> -        iov[i].iov_len = fmt->i_width * 3;
> -        src += picture->p->i_pitch;
> -    }
> -
> -    struct msghdr hdr = {
> -        .msg_name = NULL,
> -        .msg_namelen = 0,
> -        .msg_iov = iov,
> -        .msg_iovlen = iovcnt,
> -        .msg_control = NULL,
> -        .msg_controllen = 0,
> -        .msg_flags = 0 };
> -
> -    result = sendmsg(sys->fd, &hdr, 0);
> -    if (result < 0)
> -        msg_Err(vd, "sendmsg: error %s in vout display flaschen", 
> vlc_strerror_c(errno));
> -    else if (result < (int)(header_len + fmt->i_width * fmt->i_height 
> * 3))
> -        msg_Err(vd, "sendmsg only sent %d bytes in vout display 
> flaschen", result);
> +        /* send only as many rows at a time that fit into a packet */
> +        const int send_h = (rows < max_send_height) ? rows : 
> max_send_height;

Maybe use __MAX instead:
   const int send_h = __MAX(rows, max_send_height);

> +        int header_len = snprintf(header_buffer, 
> sizeof(header_buffer),
> +                                  "P6\n%d %d\n#FT: %d %d %d\n255\n",
> +                                  fmt->i_width, send_h,
> +                                  offset.x, offset.y, offset.z);

Don’t you lack checking if header_len < sizeof(header_buffer), to know 
that it
actually wrote the full header to the buffer?
Additionally you probably should check it is not negative, in case some 
error happened?

Else you would use that possibly incorrect value for the iovec below.

> +        /* Note the protocol doesn't include any picture order field. 
> */
> +        /* (maybe add as comment?) */
> +
> +        int iovcnt = 1 + send_h;
> +        if (unlikely(iovcnt > iovmax))
> +            return;
> +
> +        struct iovec iov[iovcnt];
> +        iov[0].iov_base = header_buffer;
> +        iov[0].iov_len = header_len;
> +
> +        for (int i = 1; i < iovcnt; i++)
> +        {
> +            iov[i].iov_base = picture_buffer;
> +            iov[i].iov_len = row_size;
> +            picture_buffer += picture->p->i_pitch;
> +        }
> +
> +        struct msghdr hdr = {
> +            .msg_name = NULL,
> +            .msg_namelen = 0,
> +            .msg_iov = iov,
> +            .msg_iovlen = iovcnt,
> +            .msg_control = NULL,
> +            .msg_controllen = 0,
> +            .msg_flags = 0 };
> +
> +        int result = sendmsg(sys->fd, &hdr, 0);
> +        if (result < 0)
> +            msg_Err(vd, "sendmsg: error %s in vout display flaschen", 
> vlc_strerror_c(errno));

Mentioning the „vout display flaschen“ part is kind of redundant as 
the log message context
anyway has the module name, file and line info.

> +        else if (result < (int)(header_len + fmt->i_width * send_h * 
> 3))
> +            msg_Err(vd, "sendmsg only sent %d bytes in vout display 
> flaschen", result);

Same remark as above.

> +
> +        rows -= send_h;
> +        offset.y += send_h;
>          /* we might want to drop some frames? */
> +    }
>  }
>  /**