Re: [libav-devel] [PATCH] h264dec: fix dropped initial SEI recovery point

2017-02-18 Thread John Stebbins 
On 02/10/2017 02:33 AM, Anton Khirnov wrote:
> Quoting John Stebbins (2017-02-08 22:52:28)
>> ---
>>  libavcodec/h264dec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
>> index 5137039..6d7aa7b 100644
>> --- a/libavcodec/h264dec.c
>> +++ b/libavcodec/h264dec.c
>> @@ -452,7 +452,6 @@ void ff_h264_flush_change(H264Context *h)
>>  if (h->cur_pic_ptr)
>>  h->cur_pic_ptr->reference = 0;
>>  h->first_field = 0;
>> -ff_h264_sei_uninit(>sei);
>>  h->recovery_frame = -1;
>>  h->frame_recovered = 0;
>>  }
>> @@ -466,6 +465,7 @@ static void flush_dpb(AVCodecContext *avctx)
>>  memset(h->delayed_pic, 0, sizeof(h->delayed_pic));
>>  
>>  ff_h264_flush_change(h);
>> +ff_h264_sei_uninit(>sei);
>>  
>>  for (i = 0; i < H264_MAX_PICTURE_COUNT; i++)
>>  ff_h264_unref_picture(h, >DPB[i]);
>> -- 
>> 2.9.3
> Looks good to me, but I'd appreciate Janne's opinion as well.
>

Still waiting on feedback from Janne?

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] matroskaenc: factor ts_offset in block timecode computation

2017-02-18 Thread John Stebbins 
On 02/16/2017 10:43 AM, John Stebbins wrote:
> On 02/16/2017 10:19 AM, Vittorio Giovara wrote:
>> On Thu, Feb 16, 2017 at 11:14 AM, John Stebbins  
>> wrote:
>>> On 02/15/2017 10:09 PM, Luca Barbato wrote:
 On 15/02/2017 23:29, John Stebbins wrote:
> ts_offset was added to cluster timecode, but then effectively subtracted
> back off the block timecode
> ---
>  libavformat/matroskaenc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index e951a0f..2fe6e0e 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1461,6 +1461,7 @@ static void mkv_write_block(AVFormatContext *s, 
> AVIOContext *pb,
>  uint8_t *data = NULL;
>  int offset = 0, size = pkt->size;
>  int64_t ts = mkv->tracks[pkt->stream_index].write_dts ? pkt->dts : 
> pkt->pts;
> +ts += mkv->tracks[pkt->stream_index].ts_offset;
>
>  av_log(s, AV_LOG_DEBUG, "Writing block at offset %" PRIu64 ", size 
> %d, "
> "pts %" PRId64 ", dts %" PRId64 ", duration %" PRId64 ", 
> flags %d\n",
>
 What does it fix? should it go in stable?


>>> When setting initial_padding for an audio stream, the timestamps are 
>>> written incorrectly to the mkv file.
>>> cluster timecode gets written as pts0 + ts_offset which is correct, but 
>>> then block timecode gets written as
>>> pts - cluster timecode which expanded is pts - (pts0 + ts_offset) .  Adding 
>>> cluster and block tc back together
>>> cluster tc + block tc = (pts0 + ts_offset) + (pts - (pts0 + ts_offset)) = 
>>> pts
>>> But the result should be pts + ts_offset since demux will subtract the 
>>> CodecDelay element from pts
>>> and set initial_padding to CodecDelay. This patch gives the correct result.
>> Bonus points if this comment is added verbatim to the commit log.
>

So, with these commit message updates, is this ok to commit to both master and 
stable?

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-18 Thread John Stebbins 
On 01/14/2017 12:46 PM, John Stebbins wrote:
> On 01/14/2017 12:30 PM, John Stebbins wrote:
>> On 01/12/2017 10:51 PM, wm4 wrote:
>>> On Thu, 12 Jan 2017 10:33:28 -0700
>>> John Stebbins  wrote:
>>>
 ---
  libavformat/dv.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

 diff --git a/libavformat/dv.c b/libavformat/dv.c
 index d4e5180..3ff369b 100644
 --- a/libavformat/dv.c
 +++ b/libavformat/dv.c
 @@ -484,10 +484,15 @@ static int dv_read_packet(AVFormatContext *s, 
 AVPacket *pkt)
  size = avpriv_dv_get_packet(c->dv_demux, pkt);
  
  if (size < 0) {
 +int result;
 +
  if (!c->dv_demux->sys)
  return AVERROR(EIO);
  size = c->dv_demux->sys->frame_size;
 -if (avio_read(s->pb, c->buf, size) <= 0)
 +result = avio_read(s->pb, c->buf, size);
 +if (result == AVERROR_EOF)
 +return result;
 +if (result <= 0)
  return AVERROR(EIO);
  
  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
>>> Why not just return the error code as-is?
>>>
>>> While I have my doubts whether it's useful, it's simpler and is what
>>> most other demuxers (probably) do.
>>>
>> Ok by me.  I was just making the minimal change to behaviour possible in 
>> case the current behaviour mattered to somebody.
>>
> After a closer look, is avio_read guaranteed to return AVERROR_EOF upon end 
> of input for all types of AVIOContext?  I
> see some code that explicitly checks for avio_read returning 0 and treating 
> that as EOF. I.e. how should I handle a
> return value of 0 from avio_read?
>

I forgot this was still pending.  I never got an answer to my last question 
above.  Do you all think wm4's suggestion is
sensible. His suggestion results in dv_read_packet returning 0 instead of EIO 
when avio_read returns 0. IMO, if this
change results in other issues, those issues should be fixed since they rely on 
broken behaviour, so my opinion is wm4's
suggestion is good.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-18 Thread Luca Barbato 
On 18/02/2017 14:37, John Stebbins wrote:

> I forgot this was still pending.  I never got an answer to my last
> question above.  Do you all think wm4's suggestion is sensible. His
> suggestion results in dv_read_packet returning 0 instead of EIO when
> avio_read returns 0. IMO, if this change results in other issues,
> those issues should be fixed since they rely on broken behaviour, so
> my opinion is wm4's suggestion is good.
> 

There is a pb->eof_reached for it.

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] matroskaenc: factor ts_offset in block timecode computation

2017-02-18 Thread Luca Barbato 
On 18/02/2017 14:24, John Stebbins wrote:

> So, with these commit message updates, is this ok to commit to both
> master and stable?

I guess.

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] lavc: make sure not to return EAGAIN from codecs

2017-02-18 Thread Anton Khirnov 
Quoting wm4 (2017-02-17 07:33:14)
> On Thu, 16 Feb 2017 17:34:30 +0100
> Anton Khirnov  wrote:
> 
> > This error is treated specially by the API.
> > 
> > CC: libav-sta...@libav.org
> > ---
> >  libavcodec/aacdec.c | 6 +++---
> >  libavcodec/nvenc.c  | 8 +---
> >  libavcodec/qsv.c| 8 +---
> >  3 files changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libavcodec/aacdec.c b/libavcodec/aacdec.c
> > index 22ebcdc..76c1b16 100644
> > --- a/libavcodec/aacdec.c
> > +++ b/libavcodec/aacdec.c
> > @@ -3228,7 +3228,7 @@ static int read_audio_mux_element(struct LATMContext 
> > *latmctx,
> >  } else if (!latmctx->aac_ctx.avctx->extradata) {
> >  av_log(latmctx->aac_ctx.avctx, AV_LOG_DEBUG,
> > "no decoder config found\n");
> > -return AVERROR(EAGAIN);
> > +return 1;
> 
> The 1 looks odd here, why not 0? Though it seems the return value is
> discarded if it's not negative anyway.

To distinguish it from 0, which is the "parsing successful" return
value.

> 
> >  }
> >  if (latmctx->audio_mux_version_A == 0) {
> >  int mux_slot_length_bytes = read_payload_length_info(latmctx, gb);
> > @@ -3265,8 +3265,8 @@ static int latm_decode_frame(AVCodecContext *avctx, 
> > void *out,
> >  if (muxlength > avpkt->size)
> >  return AVERROR_INVALIDDATA;
> >  
> > -if ((err = read_audio_mux_element(latmctx, )) < 0)
> > -return err;
> > +if ((err = read_audio_mux_element(latmctx, )))
> > +return (err < 0) ? err : avpkt->data;
> 
> Wait a moment, you return a pointer as an int?

Yes, I'm dumb. Changed to avpkt->size locally.

> 
> >  
> >  if (!latmctx->initialized) {
> >  if (!avctx->extradata) {
> > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> > index 90b9d1a..e8cf4b4 100644
> > --- a/libavcodec/nvenc.c
> > +++ b/libavcodec/nvenc.c
> > @@ -123,12 +123,14 @@ static const struct {
> >  { NV_ENC_ERR_OUT_OF_MEMORY,AVERROR(ENOMEM),  "out of 
> > memory"},
> >  { NV_ENC_ERR_ENCODER_NOT_INITIALIZED,  AVERROR(EINVAL),  "encoder not 
> > initialized"  },
> >  { NV_ENC_ERR_UNSUPPORTED_PARAM,AVERROR(ENOSYS),  "unsupported 
> > param"},
> > -{ NV_ENC_ERR_LOCK_BUSY,AVERROR(EAGAIN),  "lock busy"   
> >  },
> > +{ NV_ENC_ERR_LOCK_BUSY,AVERROR(EBUSY),   "lock busy"   
> >  },
> >  { NV_ENC_ERR_NOT_ENOUGH_BUFFER,AVERROR(ENOBUFS), "not enough 
> > buffer"},
> >  { NV_ENC_ERR_INVALID_VERSION,  AVERROR(EINVAL),  "invalid 
> > version"  },
> >  { NV_ENC_ERR_MAP_FAILED,   AVERROR(EIO), "map failed"  
> >  },
> > -{ NV_ENC_ERR_NEED_MORE_INPUT,  AVERROR(EAGAIN),  "need more 
> > input"  },
> > -{ NV_ENC_ERR_ENCODER_BUSY, AVERROR(EAGAIN),  "encoder 
> > busy" },
> > +/* this is error should always be treated specially, so this "mapping"
> > + * is for completeness only */
> > +{ NV_ENC_ERR_NEED_MORE_INPUT,  AVERROR_UNKNOWN,  "need more 
> > input"  },
> > +{ NV_ENC_ERR_ENCODER_BUSY, AVERROR(EBUSY),   "encoder 
> > busy" },
> >  { NV_ENC_ERR_EVENT_NOT_REGISTERD,  AVERROR(EBADF),   "event not 
> > registered" },
> >  { NV_ENC_ERR_GENERIC,  AVERROR_UNKNOWN,  "generic 
> > error"},
> >  { NV_ENC_ERR_INCOMPATIBLE_CLIENT_KEY,  AVERROR(EINVAL),  "incompatible 
> > client key"  },
> > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
> > index ab48bb0..735e153 100644
> > --- a/libavcodec/qsv.c
> > +++ b/libavcodec/qsv.c
> > @@ -94,15 +94,17 @@ static const struct {
> >  { MFX_ERR_LOCK_MEMORY,  AVERROR(EIO),"failed to lock 
> > the memory block"  },
> >  { MFX_ERR_NOT_INITIALIZED,  AVERROR_BUG, "not initialized" 
> >  },
> >  { MFX_ERR_NOT_FOUND,AVERROR(ENOSYS), "specified object 
> > was not found"   },
> > -{ MFX_ERR_MORE_DATA,AVERROR(EAGAIN), "expect more data 
> > at input"},
> > -{ MFX_ERR_MORE_SURFACE, AVERROR(EAGAIN), "expect more 
> > surface at output"},
> > +/* the following 3 errors should always be handled explicitly, so 
> > those "mappings"
> > + * are for completeness only */
> > +{ MFX_ERR_MORE_DATA,AVERROR_UNKNOWN, "expect more data 
> > at input"},
> > +{ MFX_ERR_MORE_SURFACE, AVERROR_UNKNOWN, "expect more 
> > surface at output"},
> > +{ MFX_ERR_MORE_BITSTREAM,   AVERROR_UNKNOWN, "expect more 
> > bitstream at output"  },
> >  { MFX_ERR_ABORTED,  AVERROR_UNKNOWN, "operation 
> > aborted"},
> 
> Why not AVERROR_EXTERNAL?

Because it doesn't exist and as the above comment says, the value
doesn't matter

Re: [libav-devel] [PATCH] h264_sei: Check actual presence of picture timing SEI message

2017-02-18 Thread Anton Khirnov 
Quoting Vittorio Giovara (2017-02-15 17:34:52)
> From: Michael Niedermayer 
> 
> Signed-off-by: Michael Niedermayer 
> ---
> This should hopefully be a better fix for the undetected interlaced
> samples I reported a couple of days ago.
> Vittorio

LGTM

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] h264dec: fix dropped initial SEI recovery point

2017-02-18 Thread Anton Khirnov 
Quoting John Stebbins (2017-02-18 14:25:53)
> On 02/10/2017 02:33 AM, Anton Khirnov wrote:
> > Quoting John Stebbins (2017-02-08 22:52:28)
> >> ---
> >>  libavcodec/h264dec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> >> index 5137039..6d7aa7b 100644
> >> --- a/libavcodec/h264dec.c
> >> +++ b/libavcodec/h264dec.c
> >> @@ -452,7 +452,6 @@ void ff_h264_flush_change(H264Context *h)
> >>  if (h->cur_pic_ptr)
> >>  h->cur_pic_ptr->reference = 0;
> >>  h->first_field = 0;
> >> -ff_h264_sei_uninit(>sei);
> >>  h->recovery_frame = -1;
> >>  h->frame_recovered = 0;
> >>  }
> >> @@ -466,6 +465,7 @@ static void flush_dpb(AVCodecContext *avctx)
> >>  memset(h->delayed_pic, 0, sizeof(h->delayed_pic));
> >>  
> >>  ff_h264_flush_change(h);
> >> +ff_h264_sei_uninit(>sei);
> >>  
> >>  for (i = 0; i < H264_MAX_PICTURE_COUNT; i++)
> >>  ff_h264_unref_picture(h, >DPB[i]);
> >> -- 
> >> 2.9.3
> > Looks good to me, but I'd appreciate Janne's opinion as well.
> >
> 
> Still waiting on feedback from Janne?

Yes. I'll push it this week if I don't hear anything.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel