1020679 - MP4Sample::duration should not always be 0

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

MP4Sample::duration is always 0, it should not be. This causes the BlankDecoderModule to not play audio, as it relies on the duration being correct. It should play an a4 tone.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix MP4 demuxer duration

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Does it break anything other than the blank decoder?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Anything that calls MediaData::GetEndTime() is liable to break. We do that a bit:
http://mxr.mozilla.org/mozilla-central/search?string=GetEndTime%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8435521 [details] [diff] [review]
Fix MP4 demuxer duration

Review of attachment 8435521 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +2773,5 @@
> +    mTrackFragmentHeaderInfo.mSampleDescriptionIndex = 0;
> +    mTrackFragmentHeaderInfo.mDefaultSampleDuration = 0;
> +    mTrackFragmentHeaderInfo.mDefaultSampleSize = 0;
> +    mTrackFragmentHeaderInfo.mDefaultSampleFlags = 0;
> +    mTrackFragmentHeaderInfo.mDataOffset = 0;

This just fixes an unrelated issue with uninitialised data.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8435521 [details] [diff] [review]
Fix MP4 demuxer duration

Review of attachment 8435521 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +3173,5 @@
>              mBuffer->meta_data()->setInt64(kKey64BitFileOffset, offset);
>              mBuffer->meta_data()->setInt64(
>                      kKeyTime, ((int64_t)cts * 1000000) / mTimescale);
> +            mBuffer->meta_data()->setInt64(
> +                    kKeyDuration, ((int64_t)duration * 1000000) / mTimescale);

Can mTimescale ever be 0? Please figure out whether it can be 0, and if so prevent it being 0 (in another bug/patch).

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Guard against MP4 /0 for timescale

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8438915 [details] [diff] [review]
Guard against MP4 /0 for timescale

Review of attachment 8438915 [details] [diff] [review]:
-----------------------------------------------------------------

Please also check for or otherwise prevent divisions by Track->timescale and MPEG4Extractor::mHeaderTimescale in MPEG4Extractor.cpp.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Guard against MP4 /0 for timescale

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://tbpl.mozilla.org/?tree=Try&rev=7da4d31348b4

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Crash test for timescale /0

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8438952 [details] [diff] [review]
Guard against MP4 /0 for timescale

Review of attachment 8438952 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the fix mentioned.

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +448,5 @@
>                  if (track->sampleTable->findThumbnailSample(&sampleIndex) == OK
>                          && track->sampleTable->getMetaDataForSample(
>                              sampleIndex, NULL /* offset */, NULL /* size */,
>                              &sampleTime) == OK) {
> +                    if (!track->timescale) {

The callsite of this also needs(MP4Demuxer::Init()) to null check the return value of this function before dereferencing it.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Chris Pearce (:cpearce) from comment #11)
> The callsite of this also needs(MP4Demuxer::Init()) to null check the return
> value of this function before dereferencing it.

Are you talking about:

    if (!metaData.get() || !metaData->findCString(kKeyMIMEType, &mimeType)) {
      continue;
    }

metaData.get() calls the sp::get() function. The Mozilla conventional `!metaData` doesn't compile so perhaps it be better written as:

    if (metaData == nullptr || !metaData->findCString(kKeyMIMEType, &mimeType)) {
      continue;
    }

This compiles and is a little clearer.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

That's fine.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Guard against MP4 /0 for timescale

Carrying r+

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Crash test for timescale /0

Removed b2g from crashtest based on try results; carrying r+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44f45a1e37bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0520a29947
https://hg.mozilla.org/integration/mozilla-inbound/rev/7513a64f6c1b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44f45a1e37bc
https://hg.mozilla.org/mozilla-central/rev/1f0520a29947
https://hg.mozilla.org/mozilla-central/rev/7513a64f6c1b