2.6/2.7 AudioCopy Duping

[KoolAidMan]

For the 2 first use cases you are right, long time since i worked on that part

But i'm still not getting it for the 3rd

Let's say you have video like this
XXXXXXXABCDEFGxxxxxxx
and audio like this
YXXXXXXXABCDEFGxxxxxxx

So what you want it to send the audio a little bit in the past to  resync audio and video, you end up with shift=-20 in audioCopy
The current code is dropping "Y", so that it matches


Now if it was like this

Video : XXXXXXXABCDEFGxxxxxxx
Audio : YXXXXABCDEFGxxxxxxx

i.e. ending with a positive shift, to send audio in the future, i would agree
we need to duplicate Y to end up with

Video : XXXXXXXABCDEFGxxxxxxx
Audio : YYYXXXXABCDEFGxxxxxxx

Sorry to be thick; but i'm still stuck at the sign part. There must be something obvious i'm missing
(and thanks for your patience)

We reverse the sign of the shift when converting from ms to us before it is sent to that function. See:

Code Select Expand


ADM_audioStream *audioCreateCopyStream(uint64_t startTime,int32_t shift,ADM_audioStream *input)
{
  shift*=-1000; // ms -> us


It is confusing, but in the function I am changing, a positive shift will send audio into the future and a negative shift will send it into the past.

In the createCopyAudioStream method (before we convert from ms to us), a positive shift will send audio into the past and a negative shift will send it into the future.

[mean]

Sure but the number i'm talking about are the one inside the ADMaudioCopy() part

[KoolAidMan]

Code Select Expand


ADM_audioStream *audioCreateCopyStream(uint64_t startTime,int32_t shift,ADM_audioStream *input)
{
  shift*=-1000; // ms -> us
  // fixup startTime and shift
  if(shift>0)
  {
        startTime+=shift;
        shift=0;
  }
  else //positive audio shift
  {
      int64_t comp=-shift;
      if(comp<startTime)
      {
          startTime-=comp;
          shift=0;
      }else //shifting beyond the beginning boundary of the audio stream
      {
          shift-=startTime; //do whatever you can to reduce the amount of shift
          startTime=0; //start at the beginning of the stream
      }
  }
  ADM_info("Creating audio stream copy with compensation : startTime=%s\n",ADM_us2plain(startTime));
  ADM_info("and shift =%s\n",ADM_us2plain(shift));
  return new ADM_audioStreamCopy(input,startTime,shift); //this will be the audio access
}


Code Select Expand


ADM_audioStreamCopy::ADM_audioStreamCopy(ADM_audioStream *input,uint64_t startTime, int64_t shift) :
                    ADM_audioStream(NULL,NULL)
{
    in=input;
    this->startTime=startTime;
    in->goToTime(startTime);
    this->shift=shift;
    this->dupe=false;
}

They eventually become the same number, do they not?

[mean]

Please Forget  about what happen before regarding *-1000 etc ,  and take my examples looking only at the shift value at ADM_audioStreamCopy::ADM_audioStreamCopy
i.e. if shift <0 , send packets in the past,  drop the 1st one
if shift >0, send packets in the future, duplicate potentially the beginning

Do you agree with the use case ?

[KoolAidMan]

I'm sorry but I still think this is incorrect.

i.e. if shift <0 , send packets in the past,  drop the 1st one

if shift <0, then we need to dupe until corrected dts is > 0. shift <0 means we send packets into the future.

if shift >0, send packets in the future, duplicate potentially the beginning

That function should never have shift >0 unless I set it after audio frame duplication. If we are sending packets into the past, shift will be 0 and we will have changed the startTime (and skipped to the startTime in the constructor).

[mean]

if shift = -20,  a dts=400 becomes  380 so it goes in the past

[KoolAidMan]

if shift = -20,  a dts=400 becomes  380 so it goes in the past

Yes, that looks right to me. Reminder that my code has not changed that functionality. That is how it has always worked in the copyAudioStream. My code only changes the case where corrected dts is negative.

If corrected dts=-20 we do not write back to the dts because we will enter the duping branch which purposfully does not write back.

[mean]

Ok, obviously at some point we dont understand each other
Back to the roots and see at what point it diverges :

[1]

* DTS is the timestamp at which the audio will be decoded and played, for audio PTS=DTS is assumed
*  To keep audio sync we want to have the PTS of video = PTS (=DTS) of audio , i.e. lipsync

[2]
If we take my example, audio is slightly late to video
Video = XXXXABCDXXXX
Audio = XXXXXABCDXXXX
we need to send packets of audio slightly in the past, i.e. reduce there DTS/PTS and hence apply a negative shif in ADM_audioCopy (whatever the way it ends up <0 in ADM_audioCopy)

Do we agree on [1] & [2] ?

[KoolAidMan]

Ok, obviously at some point we dont understand each other
Back to the roots and see at what point it diverges :

[1]

* DTS is the timestamp at which the audio will be decoded and played, for audio PTS=DTS is assumed
*  To keep audio sync we want to have the PTS of video = PTS (=DTS) of audio , i.e. lipsync

[2]
If we take my example, audio is slightly late to video
Video = XXXXABCDXXXX
Audio = XXXXXABCDXXXX
we need to send packets of audio slightly in the past, i.e. reduce there DTS/PTS and hence apply a negative shif in ADM_audioCopy (whatever the way it ends up <0 in ADM_audioCopy)

Do we agree on [1] & [2] ?

I agree with a caveat. There are two different scenarios.

need to shift audio into the future when startTime > shift (before ms->us)
Assume that we are copying ABCD for this video segment.
Video = XXXXABCDXXXX
Audio =   XXXXABCDXXXX

need to shift audio into the future when startTime < shift (before ms->us)
Assume that we are copying ABCD for this video segment.
Video = ABCDXXXXXXXX
Audio = AABCDXXXXXXX

[mean]

Agreed
In that case , the shift is >0 right ?

[KoolAidMan]

In the top case, the shift is 0.

In the bottom case, the shift is initially <0 (without writeback to dts), but after audio frame duplication it is >0 (with writeback to dts).

[mean]

Ok, i think i got it, it's a question of definition

[1] Mine   : The shift is global and >0. How you do frame duplication is a separate issue
[2] Yours : In the patch you posted, you managed the bottom case/frame dupe  by changing the shift, so it changes value over time

Agreed on that ?

BTW, go to time is not accurate

[KoolAidMan]

[2] Yours : In the patch you posted, you managed the bottom case/frame dupe  by changing the shift, so it changes value over time

Agreed on that ?

That is a fair summary of mine, I think.

BTW, go to time is not accurate

Yes, I noticed in MpegTS and MpegPS goToTime loops through to the next largest audio frame. I do not know how other demuxers handle it, but that shouldn't necessarily be a problem right?

Ok, i think i got it, it's a question of definition

[1] Mine   : The shift is global and >0. How you do frame duplication is a separate issue

In your code, there was also a bug--

Code Select Expand


if(corrected<startTime) goto again; // cant have <0 dts
*dts=corrected-startTime;


Since corrected is an int64_t and startTime is a uint64_t, if corrected is <0 it will be cast to a very large uint64_t and will not goto again. We would then write back the very large number to dts.

[mean]

At that point corrected is = dts, so it cannot be negative (ADM_NO_PTS has been ruled out)
And it cannot be 2^63, it s way too large

[KoolAidMan]

At that point corrected is = dts, so it cannot be negative (ADM_NO_PTS has been ruled out)
And it cannot be 2^63, it s way too large

In the existing code, it was very easy to set dts to 2^63. dts=0 and shift=-500. corrected=-500.

If((int64_t)-500<(uint64_t)0) goto again; //we would not goto again.
(uint64_t)*dts=(int64_t)-500-(uint64_t)0;