1356530 - Change the type of MediaData::mTime to TimeUnit

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1356530 - Change the type of MediaData::mTime to TimeUnit since int64_t is ambiguous.

Review commit: https://reviewboard.mozilla.org/r/131820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131820/

Comment 2

[Tzuhao Kuo [:kaku]]

Comment on attachment 8859830 [details]
Bug 1356530 - Change the type of MediaData::mTime to TimeUnit since int64_t is ambiguous.

https://reviewboard.mozilla.org/r/131820/#review134726

::: dom/media/gtest/TestMP4Demuxer.cpp:66
(Diff revision 1)
>      int64_t time = -1;
>      while (mIndex < mSamples.Length()) {
>        uint32_t i = mIndex++;
>        if (mSamples[i]->mKeyframe) {
> -        time = mSamples[i]->mTime;
> +        time = mSamples[i]->mTime.ToMicroseconds();
>          break;
>        }
>      }
>  
>      RefPtr<GenericPromise> p = mCheckTrackKeyFramePromise.Ensure(__func__);
>  
>      if (time == -1) {
>        mCheckTrackKeyFramePromise.Resolve(true, __func__);
>        return p;
>      }
>  
>  
>      DispatchTask(
>        [track, time, binding] () {
>          track->Seek(media::TimeUnit::FromMicroseconds(time))->Then(binding->mTaskQueue, __func__,
>            [track, time, binding] () {
>              track->GetSamples()->Then(binding->mTaskQueue, __func__,
>                [track, time, binding] (RefPtr<MediaTrackDemuxer::SamplesHolder> aSamples) {
> -                EXPECT_EQ(time, aSamples->mSamples[0]->mTime);
> +                EXPECT_EQ(time, aSamples->mSamples[0]->mTime.ToMicroseconds());

Make the `time` variable as a `TimeUnit` here might be better.

::: dom/media/mediasink/VideoSink.cpp:368
(Diff revision 1)
> -    int64_t frameTime = frame->mTime;
> +    int64_t frameTime = frame->mTime.ToMicroseconds();
>      if (frameTime < 0) {

`frameTime` is only used here, we can save this temporal variable.

::: dom/media/mediasink/VideoSink.cpp:377
(Diff revision 1)
> -      int64_t delta = frame->mTime - aClockTime;
> +      int64_t delta = frame->mTime.ToMicroseconds() - aClockTime;
>        t = aClockTimeStamp +
>            TimeDuration::FromMicroseconds(delta / params.mPlaybackRate);

Make the `delta` as a `TimeUnit` might be better here.

::: dom/media/platforms/agnostic/VorbisDecoder.cpp:144
(Diff revision 1)
>    MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());
>  
>    const unsigned char* aData = aSample->Data();
>    size_t aLength = aSample->Size();
>    int64_t aOffset = aSample->mOffset;
> -  int64_t aTstampUsecs = aSample->mTime;
> +  int64_t aTstampUsecs = aSample->mTime.ToMicroseconds();

*Not belong to this bug's scope.*

We don't need the `aTstampUsecs` variable, it's only used in line 213.

::: dom/media/platforms/agnostic/WAVDecoder.cpp:82
(Diff revision 1)
> -  uint64_t aTstampUsecs = aSample->mTime;
> +  uint64_t aTstampUsecs = aSample->mTime.ToMicroseconds();
>  
>    int32_t frames = aLength * 8 / mInfo.mBitDepth / mInfo.mChannels;
>  
>    AlignedAudioBuffer buffer(frames * mInfo.mChannels);
>    if (!buffer) {
>      return DecodePromise::CreateAndReject(
>        MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__), __func__);
>    }
>    for (int i = 0; i < frames; ++i) {
>      for (unsigned int j = 0; j < mInfo.mChannels; ++j) {
>        if (mInfo.mProfile == 6) {                              //ALAW Data
>          uint8_t v = aReader.ReadU8();
>          int16_t decoded = DecodeALawSample(v);
>          buffer[i * mInfo.mChannels + j] =
>              IntegerToAudioSample<AudioDataValue>(decoded);
>        } else if (mInfo.mProfile == 7) {                       //ULAW Data
>          uint8_t v = aReader.ReadU8();
>          int16_t decoded = DecodeULawSample(v);
>          buffer[i * mInfo.mChannels + j] =
>              IntegerToAudioSample<AudioDataValue>(decoded);
>        } else {                                                //PCM Data
>          if (mInfo.mBitDepth == 8) {
>            uint8_t v = aReader.ReadU8();
>            buffer[i * mInfo.mChannels + j] =
>                UInt8bitToAudioSample<AudioDataValue>(v);
>          } else if (mInfo.mBitDepth == 16) {
>            int16_t v = aReader.ReadLE16();
>            buffer[i * mInfo.mChannels + j] =
>                IntegerToAudioSample<AudioDataValue>(v);
>          } else if (mInfo.mBitDepth == 24) {
>            int32_t v = aReader.ReadLE24();
>            buffer[i * mInfo.mChannels + j] =
>                Int24bitToAudioSample<AudioDataValue>(v);
>          }
>        }
>      }
>    }
>  
>    int64_t duration = frames / mInfo.mRate;
>  
>    return DecodePromise::CreateAndResolve(
>      DecodedData{ new AudioData(aOffset, aTstampUsecs, duration, frames,

*Not belong to this bug's scope.*

We don't need the `aTstampUsecs` variable, it's not used in this period.

::: dom/media/webm/WebMDemuxer.cpp:1143
(Diff revision 1)
>    int64_t frameTime = -1;
>  
>    mNextKeyframeTime.reset();
>  
>    MediaRawDataQueue skipSamplesQueue;
>    bool foundKeyframe = false;
>    while (!foundKeyframe && mSamples.GetSize()) {
>      RefPtr<MediaRawData> sample = mSamples.PopFront();
>      if (sample->mKeyframe) {
> -      frameTime = sample->mTime;
> +      frameTime = sample->mTime.ToMicroseconds();
>        foundKeyframe = true;
>      }
>      skipSamplesQueue.Push(sample.forget());
>    }
>    Maybe<int64_t> startTime;
>    if (skipSamplesQueue.GetSize()) {
>      const RefPtr<MediaRawData>& sample = skipSamplesQueue.First();
>      startTime.emplace(sample->mTimecode.ToMicroseconds());
>    }
>    // Demux and buffer frames until we find a keyframe.
>    RefPtr<MediaRawData> sample;
>    nsresult rv = NS_OK;
>    while (!foundKeyframe && NS_SUCCEEDED((rv = NextSample(sample)))) {
>      if (sample->mKeyframe) {
> -      frameTime = sample->mTime;
> +      frameTime = sample->mTime.ToMicroseconds();
>        foundKeyframe = true;
>      }
>      int64_t sampleTimecode = sample->mTimecode.ToMicroseconds();
>      skipSamplesQueue.Push(sample.forget());
>      if (!startTime) {
>        startTime.emplace(sampleTimecode);
>      } else if (!foundKeyframe
>                 && sampleTimecode > startTime.ref() + MAX_LOOK_AHEAD) {
>        WEBM_DEBUG("Couldn't find keyframe in a reasonable time, aborting");
>        break;
>      }
>    }
>    // We may have demuxed more than intended, so ensure that all frames are kept
>    // in the right order.
>    mSamples.PushFront(Move(skipSamplesQueue));
>  
>    if (frameTime != -1) {
>      mNextKeyframeTime.emplace(media::TimeUnit::FromMicroseconds(frameTime));

It's better to have the `frameTime` as a `TimeUnit` so you can avoid three conversions in this period of code.

::: dom/media/webm/WebMDemuxer.cpp:1249
(Diff revision 1)
> -  int64_t sampleTime;
> +  media::TimeUnit sampleTime;
>  
>    WEBM_DEBUG("TimeThreshold: %f", aTimeThreshold.ToSeconds());
>    while (!found && NS_SUCCEEDED((rv = NextSample(sample)))) {
>      parsed++;
>      sampleTime = sample->mTime;
> -    if (sample->mKeyframe && sampleTime >= aTimeThreshold.ToMicroseconds()) {
> +    if (sample->mKeyframe && sampleTime >= aTimeThreshold) {
>        found = true;
>        mSamples.Reset();
>        mSamples.PushFront(sample.forget());
>      }
>    }
>    if (NS_SUCCEEDED(rv)) {
>      SetNextKeyFrameTime();
>    }
>    if (found) {
>      WEBM_DEBUG("next sample: %f (parsed: %d)",
> -               media::TimeUnit::FromMicroseconds(sampleTime).ToSeconds(),
> +               sampleTime.ToSeconds(), parsed);
> -               parsed);

*Not belong to this bug's scope.*

We don't need the `sampelTime` variable here as long as we move the log statement up.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8859830 [details]
Bug 1356530 - Change the type of MediaData::mTime to TimeUnit since int64_t is ambiguous.

https://reviewboard.mozilla.org/r/131820/#review134726

> Make the `delta` as a `TimeUnit` might be better here.

TimeUnit only supports integer division. It will be wrong at #379.

> *Not belong to this bug's scope.*
> 
> We don't need the `aTstampUsecs` variable, it's only used in line 213.

It is true only if aSample->mTime is not changed in between #144 and #213 which can be reviewed in a new bug.

> *Not belong to this bug's scope.*
> 
> We don't need the `aTstampUsecs` variable, it's not used in this period.

Ditto.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8859830 [details]
Bug 1356530 - Change the type of MediaData::mTime to TimeUnit since int64_t is ambiguous.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131820/diff/1-2/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8babe547652
Change the type of MediaData::mTime to TimeUnit since int64_t is ambiguous. r=kaku

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8babe547652