957939 - Move Audio_Track/Video_Track out of ISOTrackMetadata

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alfredo Yang (:alfredo)]

Attachment: use_container_audio_video_flags

Remove Audio_track/Video_Track definitions since there is a similar one in ContainerWriter.h.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: use_container_audio_video_flags

Comment 2

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

Comment on attachment 8357622 [details] [diff] [review]
use_container_audio_video_flags

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

I think you should go back to using Audio_Track and Video_Track instead of using ContainerWriter::HAS_AUDIO and ContainerWriter::HAS_VIDEO.

I don't like that you change from using Audio_Track and Video_Track to using ContainerWriter::HAS_AUDIO and ContainerWriter::HAS_VIDEO because this is bad English semantics. The English words "has X" (e.g. HAS_VIDEO) describes the property of *having* an X. But the code often uses HAS_VIDEO as the name of a track, the *name of* an X, not to describe the property of *having* an X.

For example, in MediaDataBox::Generate(), you call:
  if (mTrackType & ContainerWriter::HAS_AUDIO) {
    FragmentBuffer* frag = mControl->GetFragment(ContainerWriter::HAS_AUDIO);
    // ...

This is bad English semantics because you ask the ISOControl to give you a fragment, but the name of that fragment is not a name of a fragment, it's the property of having a thing. It should be the name of a thing.

Hopefully that makes sense. It's a subtle distinction, but makes the code more readable.

Also, I don't like that in MP4AudioSampleEntry and VisualSampleEntry constructor you pass ContainerWriter::HAS_AUDIO or ContainerWriter::HAS_VIDEO to the SampleEntryBox constructor. I worry that someone may change the values of ContainerWriter::HAS_AUDIO or ContainerWriter::HAS_VIDEO without telling you, and break your code.

Comment 3

[Alfredo Yang (:alfredo)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> Comment on attachment 8357622 [details] [diff] [review]
> use_container_audio_video_flags
> 
> Review of attachment 8357622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you should go back to using Audio_Track and Video_Track instead of
> using ContainerWriter::HAS_AUDIO and ContainerWriter::HAS_VIDEO.
> 
> I don't like that you change from using Audio_Track and Video_Track to using
> ContainerWriter::HAS_AUDIO and ContainerWriter::HAS_VIDEO because this is
> bad English semantics. The English words "has X" (e.g. HAS_VIDEO) describes
> the property of *having* an X. But the code often uses HAS_VIDEO as the name
> of a track, the *name of* an X, not to describe the property of *having* an
> X.
> 
> For example, in MediaDataBox::Generate(), you call:
>   if (mTrackType & ContainerWriter::HAS_AUDIO) {
>     FragmentBuffer* frag = mControl->GetFragment(ContainerWriter::HAS_AUDIO);
>     // ...
> 
> This is bad English semantics because you ask the ISOControl to give you a
> fragment, but the name of that fragment is not a name of a fragment, it's
> the property of having a thing. It should be the name of a thing.
> 
> Hopefully that makes sense. It's a subtle distinction, but makes the code
> more readable.
> 

Got it. Randy will change the name later in bug 959021. :-)

> Also, I don't like that in MP4AudioSampleEntry and VisualSampleEntry
> constructor you pass ContainerWriter::HAS_AUDIO or
> ContainerWriter::HAS_VIDEO to the SampleEntryBox constructor. I worry that
> someone may change the values of ContainerWriter::HAS_AUDIO or
> ContainerWriter::HAS_VIDEO without telling you, and break your code.

Is it better if we move Audio_Track/Video_Track from ISOTrackMetadata.h to other muxer visible only header? Then we don't need to worry about someone changing it without notifying.

Comment 4

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

(In reply to Alfredo Yang from comment #3)
> Got it. Randy will change the name later in bug 959021. :-)

OK, thanks!

> Is it better if we move Audio_Track/Video_Track from ISOTrackMetadata.h to
> other muxer visible only header? Then we don't need to worry about someone
> changing it without notifying.

If you don't expect clients of the muxer to need to use Audio_Track and Video_Track, then moving its definition to a private header is reasonable idea.

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: move_audio_video_flags

Comment 6

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

Comment on attachment 8359084 [details] [diff] [review]
move_audio_video_flags

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

::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp
@@ +515,5 @@
>  
>  nsresult
>  TimeToSampleBox::Generate(uint32_t* aBoxSize)
>  {
> +  // We don't need time to sample table in fragment mp4.

Change "fragment mp4" to "fragmented mp4" (here and below too).

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: move_audio_video_flags

Carry r+.

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: move_audio_video_flags

Carry r+

Comment 9

[Alfredo Yang (:alfredo)]

https://tbpl.mozilla.org/?tree=Try&rev=1a611fc237ab

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a547787e0340

Comment 11

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

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