| Message ID | 20170418103125.3893-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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
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.
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 [...]
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.
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. [...]
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,
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. [...]
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,
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. [...]
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,
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 [...]
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.
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
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avcodec.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)