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

[Hendrik Leppkes]

On Thu, Oct 27, 2016 at 10:59 PM, Philip Langdale  wrote:
> On 2016-10-27 13:47, Hendrik Leppkes wrote:
>>
>> On Thu, Oct 27, 2016 at 5:09 PM, Philip Langdale 
>> wrote:
>>>
>>> On Thu, 27 Oct 2016 16:01:42 +0200
>>> Timo Rothenpieler  wrote:
>>>
 >
 > breaks fate:
 >
 > ./configure && make -j12 fate-vsynth1-msmpeg4
 > ...
 > TESTvsynth1-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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Philip Langdale]

On 2016-10-27 13:47, Hendrik Leppkes wrote:
On Thu, Oct 27, 2016 at 5:09 PM, Philip Langdale  
wrote:

On Thu, 27 Oct 2016 16:01:42 +0200
Timo Rothenpieler  wrote:


>
> breaks fate:
>
> ./configure && make -j12 fate-vsynth1-msmpeg4
> ...
> TESTvsynth1-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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Thu, Oct 27, 2016 at 5:09 PM, Philip Langdale  wrote:
> On Thu, 27 Oct 2016 16:01:42 +0200
> Timo Rothenpieler  wrote:
>
>> >
>> > breaks fate:
>> >
>> > ./configure && make -j12 fate-vsynth1-msmpeg4
>> > ...
>> > TESTvsynth1-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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Philip Langdale]

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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Nicolas George]

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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Philip Langdale]

On Thu, 27 Oct 2016 16:01:42 +0200
Timo Rothenpieler  wrote:

> > 
> > breaks fate:
> > 
> > ./configure && make -j12 fate-vsynth1-msmpeg4
> > ...
> > TESTvsynth1-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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Timo Rothenpieler]

> 
> breaks fate:
> 
> ./configure && make -j12 fate-vsynth1-msmpeg4
> ...
> TESTvsynth1-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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

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 
> ---
>  libavcodec/crystalhd.c | 480 
> +
>  libavcodec/version.h   |   2 +-
>  2 files changed, 126 insertions(+), 356 deletions(-)

breaks fate:

./configure && make -j12 fate-vsynth1-msmpeg4
...
TESTvsynth1-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
@@ -1,4 +1,5 @@
 3957ca57ac97f651c828ab00d8f0e088 *tests/data/fate/vsynth1-msmpeg4.avi
 624706 tests/data/fate/vsynth1-msmpeg4.avi
-4529fee96b8073e02974f5355e5f6c4e *tests/data/fate/vsynth1-msmpeg4.out.rawvideo
-stddev:7.98 PSNR: 30.09 MAXDIFF:  104 bytes:  7603200/  7603200
+Running DIL (3.22.0) Version
+DtsDeviceOpen: Opening HW in mode 0
+DtsDeviceOpen: Create File Failed
Test vsynth1-msmpeg4 failed. Look at tests/data/fate/vsynth1-msmpeg4.err for 
details.
make: *** [fate-vsynth1-msmpeg4] Error 1


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Philip Langdale]

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 
---
 libavcodec/crystalhd.c | 480 +
 libavcodec/version.h   |   2 +-
 2 files changed, 126 insertions(+), 356 deletions(-)

diff --git a/libavcodec/crystalhd.c b/libavcodec/crystalhd.c
index 137ed20..dca58f2 100644
--- a/libavcodec/crystalhd.c
+++ b/libavcodec/crystalhd.c
@@ -97,10 +97,6 @@
 #define OUTPUT_PROC_TIMEOUT 50
 /** Step between fake timestamps passed to hardware in units of 100ns */
 #define TIMESTAMP_UNIT 10
-/** Initial value in us of the wait in decode() */
-#define BASE_WAIT 1
-/** Increment in us to adjust wait in decode() */
-#define WAIT_UNIT 1000
 
 
 /*
@@ -110,9 +106,6 @@
 typedef enum {
 RET_ERROR   = -1,
 RET_OK  = 0,
-RET_COPY_AGAIN  = 1,
-RET_SKIP_NEXT_COPY  = 2,
-RET_COPY_NEXT_FIELD = 3,
 } CopyRet;
 
 typedef struct OpaqueList {
@@ -138,10 +131,7 @@ typedef struct {
 uint8_t *sps_pps_buf;
 uint32_t sps_pps_size;
 uint8_t is_nal;
-uint8_t output_ready;
 uint8_t need_second_field;
-uint8_t skip_next_output;
-uint64_t decode_wait;
 
 uint64_t last_picture;
 
@@ -188,42 +178,42 @@ static inline BC_MEDIA_SUBTYPE id2subtype(CHDContext 
*priv, enum AVCodecID id)
 
 static inline void print_frame_info(CHDContext *priv, BC_DTS_PROC_OUT *output)
 {
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tYBuffSz: %u\n", output->YbuffSz);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tYBuffDoneSz: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tYBuffSz: %u\n", output->YbuffSz);
+av_log(priv->avctx, AV_LOG_TRACE, "\tYBuffDoneSz: %u\n",
output->YBuffDoneSz);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tUVBuffDoneSz: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tUVBuffDoneSz: %u\n",
output->UVBuffDoneSz);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tTimestamp: %"PRIu64"\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tTimestamp: %"PRIu64"\n",
output->PicInfo.timeStamp);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tPicture Number: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tPicture Number: %u\n",
output->PicInfo.picture_number);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tWidth: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tWidth: %u\n",
output->PicInfo.width);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tHeight: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tHeight: %u\n",
output->PicInfo.height);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tChroma: 0x%03x\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tChroma: 0x%03x\n",
output->PicInfo.chroma_format);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tPulldown: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tPulldown: %u\n",
output->PicInfo.pulldown);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tFlags: 0x%08x\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tFlags: 0x%08x\n",
output->PicInfo.flags);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tFrame Rate/Res: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tFrame Rate/Res: %u\n",
output->PicInfo.frame_rate);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tAspect Ratio: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tAspect Ratio: %u\n",
output->PicInfo.aspect_ratio);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tColor Primaries: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tColor Primaries: %u\n",
output->PicInfo.colour_primaries);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tMetaData: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tMetaData: %u\n",
output->PicInfo.picture_meta_payload);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tSession Number: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tSession Number: %u\n",
output->PicInfo.sess_num);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tycom: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tycom: %u\n",
output->PicInfo.ycom);
-av_log(priv->avctx, AV_LOG_VERBOSE, "\tCustom Aspect: %u\n",
+av_log(priv->avctx, AV_LOG_TRACE, "\tCustom Aspect: %u\n",