[vlc-devel] [PATCH] video_output: add support for Flaschen-Taschen screen protocol

[François Revol]

Hi,

On 19/06/2016 08:56, Rémi Denis-Courmont wrote:
> On Sunday 19 June 2016 01:48:29 François Revol wrote:
>> cf.
>> https://github.com/hzeller/flaschen-taschen/blob/master/doc/protocols.md
>> +#define T_WIDTH N_("Width")
>> +#define LT_WIDTH N_("Video width.")
>> +
>> +#define T_HEIGHT N_("Height")
>> +#define LT_HEIGHT N_("Video height.")
> 
> Please don´t create new translated strings just for punctuation.

Oh, I didn't notice the other strings, I originally stripped them down
from vmem.c, which did have dot.

>> +
>> +    char *display =ar_InheritString(vd, "flaschen-display");
>> +    if (!display)
>> +        display =trdup("ftkleine.noise");
> 
> ??

Ah, that's the address of the panel at NoiseBridge I used as fallback. I
suppose it's better to ask for one here.

>> +static void Display(vout_display_t *vd, picture_t *picture, subpicture_t
>> *subpicture)
>> +{
>> +    vout_display_sys_t *sys =d->sys;
>> +    int result;
>> +    VLC_UNUSED(vd);
> 
> Why?

Other than bad copy-pasting?

>> +    VLC_UNUSED(subpicture);
>> +
>> +    char *buffer =alloc(1, 64 + vd->fmt.i_width * vd->fmt.i_height * 3);
>> +    if (buffer) {
>> +        int header_len =nprintf(buffer, 64, "P6\n%d %d\n255\n",
>> +                                  vd->fmt.i_width, vd->fmt.i_height);
> 
> Lack of sequence number means the other end might receive picture is incorrect 
> order. Just saying.

Well, I didn't design the protocol. Although it's really not designed to
stream across the Internet but over a LAN, and it didn't work too bad
(although I didn't really check for the amount of lost packets, it
should probably be reported for the stats).

Note I don't have access to the device anymore so I'll refrain from
doing large changes for now, unless I can find someone to test them there.

>> +            for (c =; c < vd->fmt.i_width; c++)
>> +            {
>> +                uint8_t *d =dst[c*3];
>> +                uint8_t *s =src[c*3];
>> +                d[0] =[0];
>> +                d[1] =[1];
>> +                d[2] =[2];
>> +            }
>> +            dst +=d->fmt.i_width * 3;
>> +            src +=icture->p->i_pitch;
>> +        }
> 
> Memory copy is lame. Use struct iovec.

Hmm can't find any net_ function dealing with... oh ok access/udp does
it directly with BSD calls.

I originally didn't know which order I'd have to deal with (and I'm
still not sure it's valid on big endian), and as I said, I'd rather not
touch what's working for now.

>> +    switch (query) {
>> +    case VOUT_DISPLAY_CHANGE_DISPLAY_SIZE:
>> +        /* We have to ignore what is requested */
>> +        vout_display_SendEventPicturesInvalid(vd);
> 
> Why do you invalidate?
> 
>> +        return VLC_SUCCESS;
>> +
>> +    case VOUT_DISPLAY_RESET_PICTURES:
>> +        if (sys->pool)
>> +            picture_pool_Release(sys->pool);
>> +        sys->pool =ULL;
>> +
>> +        vd->fmt.i_width  =ar_InheritInteger(vd, "flaschen-width");
>> +        vd->fmt.i_height =ar_InheritInteger(vd, "flaschen-height");
>> +        return VLC_SUCCESS;
> 
> Seems useless?

At one point I noticed the display was incorrectly using some proposed
values like 1024x768 instead of the required ones, but then I noticed I
forgot to overwrite them on setup.

I'm not sure this is needed now, I mostly copied from other sample code
that had the same constraints (fixed display size)...
Looks like aa.c does this.
Although I don't call vout_display_SendEventDisplaySize() in Open(),
maybe I should?



I'll fix the cosmetics and resubmit, and hopefully I can find someone at
NoiseBridge to test the rest.

François.