[FFmpeg-devel] avcodec/avcodec: Fix text implying single threaded decoding

Submitted by Michael Niedermayer on April 18, 2017, 10:31 a.m.

Details

Message ID 20170418103125.3893-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer April 18, 2017, 10:31 a.m.
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/avcodec.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ronald S. Bultje April 18, 2017, 10:43 a.m.
Hi,

On Tue, Apr 18, 2017 at 6:31 AM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avcodec.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index ee133712b5..2ac1523a36 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -4960,7 +4960,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
> AVSubtitle *sub,
>   *                  Unlike with older APIs, the packet is always fully
> consumed,
>   *                  and if it contains multiple frames (e.g. some audio
> codecs),
>   *                  will require you to call avcodec_receive_frame()
> multiple
> - *                  times afterwards before you can send a new packet.
> + *                  times afterwards.


Does/did this imply single-threaded decoding?

Ronald
wm4 April 18, 2017, 11:24 a.m.
On Tue, 18 Apr 2017 12:31:25 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avcodec.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index ee133712b5..2ac1523a36 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -4960,7 +4960,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
>   *                  Unlike with older APIs, the packet is always fully consumed,
>   *                  and if it contains multiple frames (e.g. some audio codecs),
>   *                  will require you to call avcodec_receive_frame() multiple
> - *                  times afterwards before you can send a new packet.
> + *                  times afterwards.
>   *                  It can be NULL (or an AVPacket with data set to NULL and
>   *                  size set to 0); in this case, it is considered a flush
>   *                  packet, which signals the end of the stream. Sending the

How did the old text imply that, and how does the new text remove this
implication? Because I can't see either.

This removes the important fact that you can't just call receive at
"any time", but that it must happen before you call send again.
Michael Niedermayer April 18, 2017, 11:30 a.m.
On Tue, Apr 18, 2017 at 06:43:07AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Apr 18, 2017 at 6:31 AM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avcodec.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index ee133712b5..2ac1523a36 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -4960,7 +4960,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
> > AVSubtitle *sub,
> >   *                  Unlike with older APIs, the packet is always fully
> > consumed,
> >   *                  and if it contains multiple frames (e.g. some audio
> > codecs),
> >   *                  will require you to call avcodec_receive_frame()
> > multiple
> > - *                  times afterwards before you can send a new packet.
> > + *                  times afterwards.
> 
> 
> Does/did this imply single-threaded decoding?

if you cannot send a new packet before you received the output of
the previous then the decoder has only one packet at a time and cannot
decode multiple in parallel.

ill add above to the commit message

[...]
wm4 April 18, 2017, 12:14 p.m.
On Tue, 18 Apr 2017 13:30:43 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Tue, Apr 18, 2017 at 06:43:07AM -0400, Ronald S. Bultje wrote:
> > Hi,
> > 
> > On Tue, Apr 18, 2017 at 6:31 AM, Michael Niedermayer <michael@niedermayer.cc  
> > > wrote:  
> >   
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/avcodec.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index ee133712b5..2ac1523a36 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -4960,7 +4960,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
> > > AVSubtitle *sub,
> > >   *                  Unlike with older APIs, the packet is always fully
> > > consumed,
> > >   *                  and if it contains multiple frames (e.g. some audio
> > > codecs),
> > >   *                  will require you to call avcodec_receive_frame()
> > > multiple
> > > - *                  times afterwards before you can send a new packet.
> > > + *                  times afterwards.  
> > 
> > 
> > Does/did this imply single-threaded decoding?  
> 
> if you cannot send a new packet before you received the output of
> the previous then the decoder has only one packet at a time and cannot
> decode multiple in parallel.
> 
> ill add above to the commit message

That's not how it works. I'm against the patch.
Michael Niedermayer April 18, 2017, 1:07 p.m.
On Tue, Apr 18, 2017 at 02:14:47PM +0200, wm4 wrote:
> On Tue, 18 Apr 2017 13:30:43 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Tue, Apr 18, 2017 at 06:43:07AM -0400, Ronald S. Bultje wrote:
> > > Hi,
> > > 
> > > On Tue, Apr 18, 2017 at 6:31 AM, Michael Niedermayer <michael@niedermayer.cc  
> > > > wrote:  
> > >   
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/avcodec.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > index ee133712b5..2ac1523a36 100644
> > > > --- a/libavcodec/avcodec.h
> > > > +++ b/libavcodec/avcodec.h
> > > > @@ -4960,7 +4960,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
> > > > AVSubtitle *sub,
> > > >   *                  Unlike with older APIs, the packet is always fully
> > > > consumed,
> > > >   *                  and if it contains multiple frames (e.g. some audio
> > > > codecs),
> > > >   *                  will require you to call avcodec_receive_frame()
> > > > multiple
> > > > - *                  times afterwards before you can send a new packet.
> > > > + *                  times afterwards.  
> > > 
> > > 
> > > Does/did this imply single-threaded decoding?  
> > 
> > if you cannot send a new packet before you received the output of
> > the previous then the decoder has only one packet at a time and cannot
> > decode multiple in parallel.
> > 
> > ill add above to the commit message
> 
> That's not how it works. I'm against the patch.

Thats not how patch review works. If you have objections, you have to
explain the issue so it can be resolved/clarified.


[...]
Nicolas George April 18, 2017, 1:23 p.m.
Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
> if you cannot send a new packet before you received the output of
> the previous

You have to TRY TO receive the output before you send a new packet. A
threaded decoder will just start the work and return EAGAIN.

Regards,
Michael Niedermayer April 18, 2017, 1:44 p.m.
On Tue, Apr 18, 2017 at 03:23:14PM +0200, Nicolas George wrote:
> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
> > if you cannot send a new packet before you received the output of
> > the previous
> 
> You have to TRY TO receive the output before you send a new packet. A

This contradicts the documentation: (and would be rather rigid design)

 * Using the API as outlined above is highly recommended. But it is also
 * possible to call functions outside of this rigid schema. For example, you can
 * call avcodec_send_packet() repeatedly without calling
 * avcodec_receive_frame(). In this case, avcodec_send_packet() will succeed
 * until the codec's internal buffer has been filled up (which is typically of
 * size 1 per output frame, after initial input), and then reject input with
 * AVERROR(EAGAIN). Once it starts rejecting input, you have no choice but to
 * read at least some output.


> threaded decoder will just start the work and return EAGAIN.

Thats true but this is not permitted by the text prior to my patch

The requiement to call avcodec_receive_frame() multiple times implies
it does not return EAGAIN because you would not call it again if it
did. (one might "mathematically" work around this but the result still
wont make sense)

For reference:

@@ -4960,7 +4960,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
  *                  Unlike with older APIs, the packet is always fully consumed,
  *                  and if it contains multiple frames (e.g. some audio codecs),
  *                  will require you to call avcodec_receive_frame() multiple
- *                  times afterwards before you can send a new packet.
+ *                  times afterwards.



[...]
Nicolas George April 18, 2017, 1:47 p.m.
Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
> This contradicts the documentation: (and would be rather rigid design)

Possible. But it has nothing to do with threading.

> > threaded decoder will just start the work and return EAGAIN.
> Thats true but this is not permitted by the text prior to my patch

Yes it is.

> The requiement to call avcodec_receive_frame() multiple times implies
> it does not return EAGAIN because you would not call it again if it
> did.

No, you would not call it again, you would first have to feed it another
packet. Still no problem at this level.

Regards,
Michael Niedermayer April 18, 2017, 2:01 p.m.
On Tue, Apr 18, 2017 at 03:47:29PM +0200, Nicolas George wrote:
> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
> > This contradicts the documentation: (and would be rather rigid design)
> 
> Possible. But it has nothing to do with threading.
> 
> > > threaded decoder will just start the work and return EAGAIN.
> > Thats true but this is not permitted by the text prior to my patch
> 
> Yes it is.
> 

> > The requiement to call avcodec_receive_frame() multiple times implies
> > it does not return EAGAIN because you would not call it again if it
> > did.
> 
> No, you would not call it again, you would first have to feed it another
> packet. Still no problem at this level.

Iam not sure you did read the text the patch changes

"... require you to call avcodec_receive_frame() multiple
 times afterwards before you can send a new packet"

this just isnt true
if a decoder returns EAGAIN you would not call it multiple times


Heres the whole again for reference:

  *                  Unlike with older APIs, the packet is always fully consumed,
  *                  and if it contains multiple frames (e.g. some audio codecs),
  *                  will require you to call avcodec_receive_frame() multiple
- *                  times afterwards before you can send a new packet.
+ *                  times afterwards.

[...]
Nicolas George April 18, 2017, 2:08 p.m.
Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
> Iam not sure you did read the text the patch changes

I am replying to the issue about threading.

If you find the documentation inconsistent, your commit message must be
changed.

Regards,
Michael Niedermayer April 18, 2017, 2:12 p.m.
On Tue, Apr 18, 2017 at 04:08:43PM +0200, Nicolas George wrote:
> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
> > Iam not sure you did read the text the patch changes
> 
> I am replying to the issue about threading.

ok, iam trying to stay with the topic of this thread which was fixing
the documentation touched by the patch.

> 
> If you find the documentation inconsistent, your commit message must be
> changed.

Sure, what commit message would you suggest ?

Thanks

[...]
wm4 April 19, 2017, 1:13 a.m.
On Tue, 18 Apr 2017 16:01:48 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Tue, Apr 18, 2017 at 03:47:29PM +0200, Nicolas George wrote:
> > Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :  
> > > This contradicts the documentation: (and would be rather rigid design)  
> > 
> > Possible. But it has nothing to do with threading.
> >   
> > > > threaded decoder will just start the work and return EAGAIN.  
> > > Thats true but this is not permitted by the text prior to my patch  
> > 
> > Yes it is.
> >   
> 
> > > The requiement to call avcodec_receive_frame() multiple times implies
> > > it does not return EAGAIN because you would not call it again if it
> > > did.  
> > 
> > No, you would not call it again, you would first have to feed it another
> > packet. Still no problem at this level.  
> 
> Iam not sure you did read the text the patch changes
> 
> "... require you to call avcodec_receive_frame() multiple
>  times afterwards before you can send a new packet"
> 
> this just isnt true
> if a decoder returns EAGAIN you would not call it multiple times
> 

The decoder is free to do whatever it pleases. It _could_ force the API
user to "receive" multiple frames immediately, or it could buffer
multiple packets/frames internally and make the user "receive" them at
a later point.

The whole point of the API is to make this data flow more flexible. The
whole point about the sentence your patch changes is that you do not
somehow feed parts of the previous packet to new "send" invocations,
like the old audio decode API did.

Feel free to improve the docs (they surely are not perfect), but I
don't think your patch does. It seems to sort of miss the point.

Patch hide | download patch | download mbox

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index ee133712b5..2ac1523a36 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -4960,7 +4960,7 @@  int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
  *                  Unlike with older APIs, the packet is always fully consumed,
  *                  and if it contains multiple frames (e.g. some audio codecs),
  *                  will require you to call avcodec_receive_frame() multiple
- *                  times afterwards before you can send a new packet.
+ *                  times afterwards.
  *                  It can be NULL (or an AVPacket with data set to NULL and
  *                  size set to 0); in this case, it is considered a flush
  *                  packet, which signals the end of the stream. Sending the