| Message ID | 20170406084451.27892-1-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Apr 06, 2017 at 10:44:48AM +0200, Nicolas George wrote: > Makes the reason of the "FIXME" comment more obvious. > Avoid name conflicts for the next commit. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > ffmpeg.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > > Unchanged. > > > diff --git a/ffmpeg.c b/ffmpeg.c > index ea03179c21..4db8ea82ac 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -2617,7 +2617,7 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo > > // while we have more to decode or while the decoder did output something on EOF > while (ist->decoding_needed) { > - int duration = 0; > + int duration_dts = 0; what is duration_dts ? [...]
Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit :
> what is duration_dts ?
It is the duration that is needed to update the DTS variables. The next
patch, as you can see, introduces duration_pts for the same task for the
PTS variables.
The current code incorrectly used the same duration for both, which is
incorrect when a packet is not decoded into a frame immediately,
possibly due to B-frames and reordering or to threading.
Regards,
On Thu, Apr 06, 2017 at 06:15:56PM +0200, Nicolas George wrote: > Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > > what is duration_dts ? > > It is the duration that is needed to update the DTS variables. The next > patch, as you can see, introduces duration_pts for the same task for the > PTS variables. > > The current code incorrectly used the same duration for both, which is > incorrect when a packet is not decoded into a frame immediately, > possibly due to B-frames and reordering or to threading. Its a while ago i worked with this and the code changed since then so maybe iam wrong but pkt->duration is the duration of the frame encoded in that packet that is not neccesarily the time between its dts and the next dts (well it is for a lot of cases of course where its guranteed but ...) example (mpeg1/mpeg2/mpeg4) I P B P B PTS 1 10 2 20 19 DTS 0 1 2 10 19 DUR 1 9 8 x 1 Its quite possible i made a mistake of course ... [...]
Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > Its a while ago i worked with this and the code changed since then > so maybe iam wrong but > > pkt->duration is the duration of the frame encoded in that packet > that is not neccesarily the time between its dts and the next dts > (well it is for a lot of cases of course where its guranteed but ...) > > example (mpeg1/mpeg2/mpeg4) > I P B P B > PTS 1 10 2 20 19 > DTS 0 1 2 10 19 > DUR 1 9 8 x 1 > > Its quite possible i made a mistake of course ... Well, I do not know about that, I always thought DTS was stupid anyway. My patch is not changing the DTS logic, only the name of a variable to reflect its usage in the current code and avoid conflict with the next patch. The next patch is the important one. If the DTS logic needs fixing and you (or anybody else) end up doing it, then the name of the variable can be fixed at the same time. And having "dts" in it will help finding it. Regards,
On Thu, Apr 06, 2017 at 07:03:25PM +0200, Nicolas George wrote: > Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > > Its a while ago i worked with this and the code changed since then > > so maybe iam wrong but > > > > pkt->duration is the duration of the frame encoded in that packet > > that is not neccesarily the time between its dts and the next dts > > (well it is for a lot of cases of course where its guranteed but ...) > > > > example (mpeg1/mpeg2/mpeg4) > > I P B P B > > PTS 1 10 2 20 19 > > DTS 0 1 2 10 19 > > DUR 1 9 8 x 1 > > > > Its quite possible i made a mistake of course ... > > Well, I do not know about that, I always thought DTS was stupid anyway. > > My patch is not changing the DTS logic, only the name of a variable to > reflect its usage in the current code and avoid conflict with the next > patch. The next patch is the important one. > > If the DTS logic needs fixing and you (or anybody else) end up doing it, > then the name of the variable can be fixed at the same time. And having > "dts" in it will help finding it. A comment explaining this (if you agree that the issue is real and not some mistake of understanding something on my side) would be a good idea. could be confusing otherwise and lead to misunderstandings [...]
Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > A comment explaining this (if you agree that the issue is real and not > some mistake of understanding something on my side) > would be a good idea. > could be confusing otherwise and lead to misunderstandings I am sorry, I have no idea what kind of comment you would like. Once again, this patch is just renaming a variable to have a name similar to how it is used. It does not change the logic of the current code, and if anything it makes it clearer. I do not think adding a comment belongs here. Regards,
On Thu, Apr 06, 2017 at 07:35:04PM +0200, Nicolas George wrote: > Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > > A comment explaining this (if you agree that the issue is real and not > > some mistake of understanding something on my side) > > would be a good idea. > > could be confusing otherwise and lead to misunderstandings > > I am sorry, I have no idea what kind of comment you would like. > > Once again, this patch is just renaming a variable to have a name > similar to how it is used. It does not change the logic of the current > code, and if anything it makes it clearer. I do not think adding a > comment belongs here. Currently the packet duration is assigned to a variable called duration After your patch it is assigned to a variable called duration_dts Someone reading duration_dts = pkt->duration; will have an idea what pkt->duration is, our stuff is often documented through code. If a line of code gives a incorrect impression, it should be documented to avoid it leading to misunderstandings. I assume here that i didnt miss something all along and this is actually buggy Also duration_dts should be documented, what does it exactly mean the difference between teh current and next dts ? if so it should probably also be changed to int64_t, thats not a issue in this patch, but trivial to fix and you already work on this. If i fix it i would break your patchset if i apply it before and ill forget to do it later ... [...]
Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > If i fix it i would break your patchset if i apply it before and ill > forget to do it later ... Do not worry about that, I will rebase and fix the conflicts if necessary. On the other hand, I would very much like to avoid wasting more time bikeshedding a cosmetic patch. Regards,
On Thu, Apr 06, 2017 at 08:56:19PM +0200, Nicolas George wrote: > Le septidi 17 germinal, an CCXXV, Michael Niedermayer a écrit : > > If i fix it i would break your patchset if i apply it before and ill > > forget to do it later ... > > Do not worry about that, I will rebase and fix the conflicts if > necessary. On the other hand, I would very much like to avoid wasting > more time bikeshedding a cosmetic patch. ok, ill fix the type then thx [...]
diff --git a/ffmpeg.c b/ffmpeg.c index ea03179c21..4db8ea82ac 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -2617,7 +2617,7 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo // while we have more to decode or while the decoder did output something on EOF while (ist->decoding_needed) { - int duration = 0; + int duration_dts = 0; int got_output = 0; int decode_failed = 0; @@ -2634,22 +2634,22 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo &decode_failed); if (!repeating || !pkt || got_output) { if (pkt && pkt->duration) { - duration = av_rescale_q(pkt->duration, ist->st->time_base, AV_TIME_BASE_Q); + duration_dts = av_rescale_q(pkt->duration, ist->st->time_base, AV_TIME_BASE_Q); } else if(ist->dec_ctx->framerate.num != 0 && ist->dec_ctx->framerate.den != 0) { int ticks= av_stream_get_parser(ist->st) ? av_stream_get_parser(ist->st)->repeat_pict+1 : ist->dec_ctx->ticks_per_frame; - duration = ((int64_t)AV_TIME_BASE * + duration_dts = ((int64_t)AV_TIME_BASE * ist->dec_ctx->framerate.den * ticks) / ist->dec_ctx->framerate.num / ist->dec_ctx->ticks_per_frame; } - if(ist->dts != AV_NOPTS_VALUE && duration) { - ist->next_dts += duration; + if(ist->dts != AV_NOPTS_VALUE && duration_dts) { + ist->next_dts += duration_dts; }else ist->next_dts = AV_NOPTS_VALUE; } if (got_output) - ist->next_pts += duration; //FIXME the duration is not correct in some cases + ist->next_pts += duration_dts; //FIXME the duration is not correct in some cases break; case AVMEDIA_TYPE_SUBTITLE: if (repeating)
Makes the reason of the "FIXME" comment more obvious. Avoid name conflicts for the next commit. Signed-off-by: Nicolas George <george@nsup.org> --- ffmpeg.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) Unchanged.