964197 - [MediaEncoder] Add video frame duration field in ISOMediaWriter

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Alfredo Yang (:alfredo)]

Video frame rate is not fixed, muxer needs to keep each frame's duration for audio/video synchronization.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: keep_each_frame_duration

Comment 2

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

Comment on attachment 8365844 [details] [diff] [review]
keep_each_frame_duration

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

Looks good.

::: content/media/encoder/fmp4_muxer/ISOControl.h
@@ +100,5 @@
>    //   It only means the fragment number of current accumulated frames, not
>    //   the current 'creating' fragment mFragNum in ISOControl.
>    uint32_t mFragmentNumber;
>  
> +  // The last frame time tamp of last fragment. It is for cauculating the

s/time tamp/time stamp/
s/cauculating/calculating/
s/fragmenet/fragment/

::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp
@@ +159,5 @@
> +          frag->SetLastFragmentLastFrameTime(frames.ElementAt(i)->GetTimeStamp());
> +        }
> +      }
> +      sample_info_table[i].sample_duration =
> +        frame_time * mMeta.mVidMeta->VideoFrequency / 1000000;

Is the 1000000 here a specific value? Say number of microseconds per second? If so, please make this a #defined constant (or static const uint32_t/uint64_t), so that it's unlikely that people will accidentally miss adding a 0 when they use write the literal value.

For example, VideoUtils.h defines USECS_PER_S as the number of microseconds per second here:
http://dxr.mozilla.org/mozilla-central/source/content/media/VideoUtils.h#122

You could use that (if the 1000000 here represents the number of microseconds per second), or you could define your own that semantically makes sense here.

Comment 3

[Alfredo Yang (:alfredo)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> For example, VideoUtils.h defines USECS_PER_S as the number of microseconds
> per second here:
> http://dxr.mozilla.org/mozilla-central/source/content/media/VideoUtils.h#122
> 
> You could use that (if the 1000000 here represents the number of
> microseconds per second), or you could define your own that semantically
> makes sense here.

 It represents microseconds per second here. I'll use USECS_PER_S in patch.
 Thanks for advice.

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: keep_each_frame_duration

Carry r+.

Comment 5

[Alfredo Yang (:alfredo)]

https://tbpl.mozilla.org/?tree=Try&rev=8ec826401ce4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a9d899c0d3

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Carsten Book [:Tomcat]]

correcting target milestone, sorry for the bugmail spam