[FFmpeg-devel] [PATCH v6 1/4] lavc: add FLIF decoding support

[Anamitra Ghorui]

On Thu, 27 Aug 2020 08:46:00 +0530
Anamitra Ghorui <aghorui at teknik.io> wrote:

> (I apologise if this mail gets sent to the wrong thread and other
> errors. I'm trying out a new mail client)
> 
> On Wed, 26 Aug 2020 16:31:46 +0000
> "Anamitra Ghorui" <aghorui at teknik.io> wrote:
> 
> [...]
> 
> > I agree that the case statements between if statements and while
> > statements are not very good looking, but this allows us to extract the
> > function logic without having to set the, or look at the state
> > management variables and statements. Trying to make do without the
> > interleaving statements would cause us to set s->segment many times,
> > set multiple switch statements etc.
> > 
> >     func(FLIF16XYZContext *s, ...)
> >     {
> >         // Init stuff
> >         ...
> >         
> >         switch (s->segment) {
> >         /* Note how cases are right before RAC calls */
> >         case 1:
> >             RAC_GET(...);
> >             ...
> >         case 2:
> >             RAC_GET(...);
> >             ...
> >         ...
> >         }
> > 
> >         // End stuff
> >         ...
> >         return 0;
> > 
> >     need more data:
> >         ...
> >         return AVERROR(EAGAIN);
> >     }
> > 
> > I get your point about using do {} while (0) to break the sequence, it
> > does make sense now to ditch the need_more_data entirely and hence free 
> > it from having to using a specific goto label, however it introduces an 
> > additional level of indentation. This is how majority of the decoder 
> > logic and transform logic has been written, so it may make the code 
> > look a bit bulkier. Should I do it anyway?
> > 
> >     func(FLIF16XYZContext *s, ...)
> >     {
> >         // Init stuff
> >         ...
> > 
> >         do {
> >             switch (s->segment) {
> >             /* Note how cases are right before RAC calls */
> >             case 1:
> >                 RAC_GET(...);
> > 
> >             case 2:
> >                 RAC_GET(...);
> >             ...
> >             }
> > 
> >             // End stuff
> >             ...
> >             return 0;
> >         } while (0);
> > 
> >         // EAGAIN stuff
> >         ...
> >         return AVERROR(EAGAIN);
> >     }
> > 
> >   
> 
> Wait a second, that's wrong. It should be like this instead:
> 
>     int main()
>     {
>         // Init Stuff
>         ...
>         switch (k) {
>         case 0:
>             RAC_GET(...)
>             break;

Please ignore the break. It shouldn't be here. you might notice from
the function name I was trying to test the structure out with sample
code.

>         ...
>         case N:
>             RAC_GET(...)
> 
>         default:
>             // Success end case
>             // We don't want the control here if error case
>             return 0;
>         }
> 
>         // Error end case
>         return AVERROR(EAGAIN);
>     }

The above code may be the better option if we decide to remove the goto
label based approach, but I would like to hear your comments on it and
some of the other mentioned things first.

--
Regards,
Anamitra