[FFmpeg-devel,02/10] crystalhd: Switch to new decode API and remove the insanity

On Wed, Oct 26, 2016 at 12:40:20PM -0700, Philip Langdale wrote:
> The new decode API allows for m:n decode patterns, which is what
> you need to use this hardware in a sane way. There are so many
> situations where 1:1 doesn't happen naturally that it's a miracle
> I got it working as well as I did.
> 
> With this change, we can throw all of the crazy heuristics and
> sleeps(!) out, and things work correctly.
> 
> Small issue - mpv (at least) expects that every output call after
> input EOF returns a frame or is the end of the stream - this is not
> behaviour we can offer - sometimes we have separate fields that
> require two calls to collect and combine.
> 
> Trying to do a tight loop in the decoder to get the second field
> requires restoring some heuristics to avoid dead-locking when
> additional input is required to get additional output.
> 
> Signed-off-by: Philip Langdale <philipl@overt.org>
> ---
>  libavcodec/crystalhd.c | 480 +++++++++++++------------------------------------
>  libavcodec/version.h   |   2 +-
>  2 files changed, 126 insertions(+), 356 deletions(-)

breaks fate:

./configure && make -j12 fate-vsynth1-msmpeg4
...
TEST    vsynth1-msmpeg4
Test vsynth1-msmpeg4 failed. Look at tests/data/fate/vsynth1-msmpeg4.err for details.
make: *** [fate-vsynth1-msmpeg4] Error 1


[...]

> 
> breaks fate:
> 
> ./configure && make -j12 fate-vsynth1-msmpeg4
> ...
> TEST    vsynth1-msmpeg4
> --- ./tests/ref/vsynth/vsynth1-msmpeg4  2016-10-27 03:11:18.675647981 +0200
> +++ tests/data/fate/vsynth1-msmpeg4     2016-10-27 15:01:15.397863504 +0200

Does fate seriously test crystalhd hardware?
I doubt this patch/series is related to that.

On Thu, 27 Oct 2016 16:01:42 +0200
Timo Rothenpieler <timo@rothenpieler.org> wrote:

> > 
> > breaks fate:
> > 
> > ./configure && make -j12 fate-vsynth1-msmpeg4
> > ...
> > TEST    vsynth1-msmpeg4
> > --- ./tests/ref/vsynth/vsynth1-msmpeg4  2016-10-27
> > 03:11:18.675647981 +0200 +++ tests/data/fate/vsynth1-msmpeg4
> > 2016-10-27 15:01:15.397863504 +0200  
> 
> Does fate seriously test crystalhd hardware?
> I doubt this patch/series is related to that.

Yeah, that's failing because it's attempting to load the decoder and
the library spams stdout (which sucks, but what can you do) with the
error messages.

I've already marked the codec as AV_CODEC_CAP_AVOID_PROBING so I don't
think there's anything else I can do.

--phil

Le sextidi 6 brumaire, an CCXXV, Philip Langdale a écrit :
> the library spams stdout (which sucks, but what can you do)

First step, look if there is a clean way of fixing it from the outside.
Maybe by setting a logging callback? You probably already did that.

Second step, if there is no clean way of fixing it from the outside:
make a bug report. You probably already did it too.

Third step, waiting for the bug to be fixed, and that maybe you did not
think of because it is ugly:

	saved_stdin = dup(1);
	dup2(2, 1);
	crystalhd_spamming_function();
	dup2(saved_stdin, 1);
	close(saved_stdin);

Regards,

On 2016-10-27 09:48, Nicolas George wrote:
> Le sextidi 6 brumaire, an CCXXV, Philip Langdale a écrit :
>> the library spams stdout (which sucks, but what can you do)
> 
> First step, look if there is a clean way of fixing it from the outside.
> Maybe by setting a logging callback? You probably already did that.
> 
> Second step, if there is no clean way of fixing it from the outside:
> make a bug report. You probably already did it too.

The code is abandoned by its creators, so yeah, these are futile.

> Third step, waiting for the bug to be fixed, and that maybe you did not
> think of because it is ugly:
> 
> 	saved_stdin = dup(1);
> 	dup2(2, 1);
> 	crystalhd_spamming_function();
> 	dup2(saved_stdin, 1);
> 	close(saved_stdin);

Horrifying.

--phil

On Thu, Oct 27, 2016 at 5:09 PM, Philip Langdale <philipl@overt.org> wrote:
> On Thu, 27 Oct 2016 16:01:42 +0200
> Timo Rothenpieler <timo@rothenpieler.org> wrote:
>
>> >
>> > breaks fate:
>> >
>> > ./configure && make -j12 fate-vsynth1-msmpeg4
>> > ...
>> > TEST    vsynth1-msmpeg4
>> > --- ./tests/ref/vsynth/vsynth1-msmpeg4  2016-10-27
>> > 03:11:18.675647981 +0200 +++ tests/data/fate/vsynth1-msmpeg4
>> > 2016-10-27 15:01:15.397863504 +0200
>>
>> Does fate seriously test crystalhd hardware?
>> I doubt this patch/series is related to that.
>
> Yeah, that's failing because it's attempting to load the decoder and
> the library spams stdout (which sucks, but what can you do) with the
> error messages.
>
> I've already marked the codec as AV_CODEC_CAP_AVOID_PROBING so I don't
> think there's anything else I can do.
>

If the decoder is being used without requesting it explicitly, then
something is wrong somewhere.
All these hardware decoders should only ever get used when the user
asks for it directly.

- Hendrik

On 2016-10-27 13:47, Hendrik Leppkes wrote:
> On Thu, Oct 27, 2016 at 5:09 PM, Philip Langdale <philipl@overt.org> 
> wrote:
>> On Thu, 27 Oct 2016 16:01:42 +0200
>> Timo Rothenpieler <timo@rothenpieler.org> wrote:
>> 
>>> >
>>> > breaks fate:
>>> >
>>> > ./configure && make -j12 fate-vsynth1-msmpeg4
>>> > ...
>>> > TEST    vsynth1-msmpeg4
>>> > --- ./tests/ref/vsynth/vsynth1-msmpeg4  2016-10-27
>>> > 03:11:18.675647981 +0200 +++ tests/data/fate/vsynth1-msmpeg4
>>> > 2016-10-27 15:01:15.397863504 +0200
>>> 
>>> Does fate seriously test crystalhd hardware?
>>> I doubt this patch/series is related to that.
>> 
>> Yeah, that's failing because it's attempting to load the decoder and
>> the library spams stdout (which sucks, but what can you do) with the
>> error messages.
>> 
>> I've already marked the codec as AV_CODEC_CAP_AVOID_PROBING so I don't
>> think there's anything else I can do.
>> 
> 
> If the decoder is being used without requesting it explicitly, then
> something is wrong somewhere.
> All these hardware decoders should only ever get used when the user
> asks for it directly.

I suspect the answer is this:

     REGISTER_DECODER(MSMPEG4_CRYSTALHD, msmpeg4_crystalhd);
     REGISTER_DECODER(MSMPEG4V1,         msmpeg4v1);
     REGISTER_ENCDEC (MSMPEG4V2,         msmpeg4v2);
     REGISTER_ENCDEC (MSMPEG4V3,         msmpeg4v3);

It's getting first priority due to declaration order. I imagine that
moving it last will avoid this test failure.

--phil

On Thu, Oct 27, 2016 at 10:59 PM, Philip Langdale <philipl@overt.org> wrote:
> On 2016-10-27 13:47, Hendrik Leppkes wrote:
>>
>> On Thu, Oct 27, 2016 at 5:09 PM, Philip Langdale <philipl@overt.org>
>> wrote:
>>>
>>> On Thu, 27 Oct 2016 16:01:42 +0200
>>> Timo Rothenpieler <timo@rothenpieler.org> wrote:
>>>
>>>> >
>>>> > breaks fate:
>>>> >
>>>> > ./configure && make -j12 fate-vsynth1-msmpeg4
>>>> > ...
>>>> > TEST    vsynth1-msmpeg4
>>>> > --- ./tests/ref/vsynth/vsynth1-msmpeg4  2016-10-27
>>>> > 03:11:18.675647981 +0200 +++ tests/data/fate/vsynth1-msmpeg4
>>>> > 2016-10-27 15:01:15.397863504 +0200
>>>>
>>>> Does fate seriously test crystalhd hardware?
>>>> I doubt this patch/series is related to that.
>>>
>>>
>>> Yeah, that's failing because it's attempting to load the decoder and
>>> the library spams stdout (which sucks, but what can you do) with the
>>> error messages.
>>>
>>> I've already marked the codec as AV_CODEC_CAP_AVOID_PROBING so I don't
>>> think there's anything else I can do.
>>>
>>
>> If the decoder is being used without requesting it explicitly, then
>> something is wrong somewhere.
>> All these hardware decoders should only ever get used when the user
>> asks for it directly.
>
>
> I suspect the answer is this:
>
>     REGISTER_DECODER(MSMPEG4_CRYSTALHD, msmpeg4_crystalhd);
>     REGISTER_DECODER(MSMPEG4V1,         msmpeg4v1);
>     REGISTER_ENCDEC (MSMPEG4V2,         msmpeg4v2);
>     REGISTER_ENCDEC (MSMPEG4V3,         msmpeg4v3);
>
> It's getting first priority due to declaration order. I imagine that
> moving it last will avoid this test failure.
>
>

Indeed. Hardware codecs should appear after the software.

- Hendrik