Closed Bug 994557 Opened 6 years ago Closed 6 years ago

Unable to flush out the WebM header only through WebMWriter.

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bechen, Assigned: bechen)

References

Details

Attachments

(2 files, 3 obsolete files)

The WebMWriter can not output the header after setting audio and video metadata.
I expected we can retrieve header after EbmlComposer::GenerateHeader is called.

/// Test case  
  // Set vorbis metadata
  WebMVorbisTrackEncoder vorbisEncoder;
  EXPECT_TRUE(vorbisEncoder.TestVorbisCreation(1, 44100));
  nsRefPtr<TrackMetadataBase> vorbisMeta = vorbisEncoder.GetMetadata();
  writer.SetMetadata(vorbisMeta);

  // Set vp8 metadata
  WebMVP8TrackEncoder vp8Encoder;
  EXPECT_TRUE(vp8Encoder.TestVP8Creation(640, 480, 640, 480, 90000));
  nsRefPtr<TrackMetadataBase> vp8Meta = vp8Encoder.GetMetadata();
  writer.SetMetadata(vp8Meta);

  writer.GetContainerData(&encodedBuf, ContainerWriter::FLUSH_NEEDED);
Blocks: 951040
There is a bug about EbmlComposer::FinishCluster I found when tracing the EbmlComposer.cpp.

ex: If the input video frame is : Iframe Iframe Iframe... So each Iframe is a cluster in WebM.
1. At the beginning, the first slot in |mClusterBuffs| is the header generated by EbmlComposer::GenerateHeader.
2. When the first Iframe is coming, the mClusterHeaderIndex will be set to 1.
3. Then the second Iframe is coming, the header and the first Iframe will be flush out.
mClusterHeaderIndex will be set to 0.
4. Then the third Iframe is coming, the second Iframe won't be flush out because mClusterHeaderIndex is 0.
http://dxr.mozilla.org/mozilla-central/source/content/media/webm/EbmlComposer.cpp#90
ah, correct. please help to fix it. thanks.
Attached patch bug-994557.patch (obsolete) — Splinter Review
1. fix the bug I mentioned at comment 1
2. Can flush out the header only without a Iframe cluster
Attachment #8405236 - Flags: review?(giles)
Comment on attachment 8405236 [details] [diff] [review]
bug-994557.patch

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

::: content/media/webm/EbmlComposer.cpp
@@ +168,1 @@
>      FinishCluster();

In this case of WriteSimpleBlock sequence.
I PPPPPPPPPPPPPPP IPPPPPPPP
         ^ call ExtractBuffer method here, 
Would it break rule of "Key frames SHOULD be placed at the beginning of clusters."?
(In reply to Randy Lin [:rlin] from comment #4)
> Comment on attachment 8405236 [details] [diff] [review]
> bug-994557.patch
> 
> Review of attachment 8405236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webm/EbmlComposer.cpp
> @@ +168,1 @@
> >      FinishCluster();
> 
> In this case of WriteSimpleBlock sequence.
> I PPPPPPPPPPPPPPP IPPPPPPPP
>          ^ call ExtractBuffer method here, 
> Would it break rule of "Key frames SHOULD be placed at the beginning of
> clusters."?
Yes, it breaks the rule if extract buffer after ContainerWriter::FLUSH_NEEDED

In the case:
Assume the I-Frame interval is 3. (3 P-Frames)
1. extract buffer with ContainerWriter::FLUSH_NEEDED
get : IPP
2. extract buffer again:
get : PIP or PIPPP
Comment on attachment 8405236 [details] [diff] [review]
bug-994557.patch

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

Looks like a reasonable direction. Please address comments and resubmit for another review.

::: content/media/webm/EbmlComposer.cpp
@@ +103,5 @@
>  
>    if (aFrame->GetFrameType() == EncodedFrame::FrameType::VP8_I_FRAME) {
>      EbmlLoc ebmlLoc;
>      Ebml_StartSubElement(&ebml, &ebmlLoc, Cluster);
> +    // current cluster header array index

This is safe because we've just called AppendElement(), but please add a MOZ_ASSERT(mClusterBuffs.Length() > 0); to catch underflow if the code changes later.

nsTArray::operator[] would also assert if the index is out of range, but it's nice to have a clean trace.

@@ +168,1 @@
>      FinishCluster();

I guess we could check if the first buffer is a keyframe and refuse to flush the cluster if it's not?

@@ +174,5 @@
>    mClusterCanFlushBuffs.Clear();
>  }
>  
>  EbmlComposer::EbmlComposer()
> +  : mClusterHeaderIndex(-1)

Using a signed int here means we can underflow, possibly leaking memory to web content. Can you use NoIndex here instead?
Attachment #8405236 - Flags: review?(giles) → feedback+
Attached patch bug-994557.v01.patch (obsolete) — Splinter Review
1. add a variable mFlushState to indicate the data types in mClusterBuffs.
enum {
  FLUSH_NONE = 0,
  FLUSH_METADATA = 1 << 0,
  FLUSH_CLUSTER = 1 << 1
  };
uint32_t mFlushState;

2. add a function FinishMetadata() to flush the metadata.
FinishMetadata() will flush metadata only,
FinishCluster() will flush metadata and clusters.

3. Correct the code flow.
3.1: Can flush metadata only.
3.2: Ensure that "Key frames SHOULD be placed at the beginning of clusters" even the ContainerWriter::FLUSH_NEEDED is called.
Attachment #8405236 - Attachment is obsolete: true
Attachment #8406603 - Flags: review?(giles)
Comment on attachment 8406603 [details] [diff] [review]
bug-994557.v01.patch

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

I think this approach better. r=me with comments addressed.

BTW, it would be nice to have the formatting and comment typo fixes in a separate patch. Makes the code changes easier to review and the commit history easier to read later.

::: content/media/webm/EbmlComposer.cpp
@@ +68,5 @@
> +void EbmlComposer::FinishMetadata()
> +{
> +  if (mFlushState & FLUSH_METADATA) {
> +    // We don't remove the first element of mClusterBuffs because the
> +    // |mClusterHeaderIndex| may have value.

Won't this produce a zero-length SimpleBlock? Can you just decrement mClusterHeaderIndex? Or assert it's zero since we're doing metadata here?

@@ +77,5 @@
>  
>  void EbmlComposer::FinishCluster()
>  {
> +  FinishMetadata();
> +  if (mFlushState & FLUSH_CLUSTER) {

Please do an early return here instead of indenting the main function body.

if (!(mFlushState & FLUSH_CLUSTER)) {
  // No data available.
  return;
}

@@ +105,5 @@
>    EbmlGlobal ebml;
>    ebml.offset = 0;
>  
> +  if (aFrame->GetFrameType() == EncodedFrame::FrameType::VP8_I_FRAME &&
> +      (mFlushState & FLUSH_CLUSTER)) {

The FLUSH_CLUSTER check is redundant here, isn't it? Metadata should already have been flushed here, and FinishCluster() is safe call when there's no data available.

I guess you've replaced the 'mClusterHeaderIndex > 0' check with mFlushState, but it's fine to trust the other method here. If you're worried about catching incorrect call sequences, you could MOZ_ASSERT or NS_WARNING instead if !(mFlushState & FLUSH_CLUSTER).

@@ +116,1 @@
>    ebml.buf = mClusterBuffs.LastElement().Elements();

Maybe this would be better as:

  auto block = mClusterBuffs.AppendElement();
  block->SetLength(aFrame->GetFrameData().Length() + DEFAULT_HEADER_SIZE);
  ebml.buf = block->Elements();

And likewise with the mClusterBuffs.LastElement().SetLength() at the end of the funciton.

@@ +124,2 @@
>      mClusterLengthLoc = ebmlLoc.offset;
>      if (aFrame->GetFrameType() != EncodedFrame::FrameType::VORBIS_AUDIO_FRAME) {

Is this ever false? We're inside an aFrame->GetFrameType() == VP8_I_FRAME block.

::: content/media/webm/EbmlComposer.h
@@ +59,5 @@
> +  enum {
> +    FLUSH_NONE = 0,
> +    FLUSH_METADATA = 1 << 0,
> +    FLUSH_CLUSTER = 1 << 1
> +  };

Since this is private (and we can't use a 'enum class' to get type-checked flags) you could put this in the .cpp file to save parser overhead. I don't have a strong opinion. Having the values with the variable declaration is also good. Just wanted to point out the possibility.
Attachment #8406603 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #8)
> Comment on attachment 8406603 [details] [diff] [review]
> bug-994557.v01.patch
> 
> Review of attachment 8406603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this approach better. r=me with comments addressed.
> 
> BTW, it would be nice to have the formatting and comment typo fixes in a
> separate patch. Makes the code changes easier to review and the commit
> history easier to read later.
> 
> ::: content/media/webm/EbmlComposer.cpp
> @@ +68,5 @@
> > +void EbmlComposer::FinishMetadata()
> > +{
> > +  if (mFlushState & FLUSH_METADATA) {
> > +    // We don't remove the first element of mClusterBuffs because the
> > +    // |mClusterHeaderIndex| may have value.
> 
> Won't this produce a zero-length SimpleBlock? Can you just decrement
> mClusterHeaderIndex? Or assert it's zero since we're doing metadata here?
> 
Yes, but the zero-length entry won't be flush out because the for-loop in EbmlComposer::FinishCluster starts from mClusterHeaderIndex.
Decrement the mClusterHeaderIndex is also fine but need to check the FLUSH_CLUSTER before decreasing.

> @@ +105,5 @@
> >    EbmlGlobal ebml;
> >    ebml.offset = 0;
> >  
> > +  if (aFrame->GetFrameType() == EncodedFrame::FrameType::VP8_I_FRAME &&
> > +      (mFlushState & FLUSH_CLUSTER)) {
> 
> The FLUSH_CLUSTER check is redundant here, isn't it? Metadata should already
> have been flushed here, and FinishCluster() is safe call when there's no
> data available.
> 
Yes, redundant.

> @@ +124,2 @@
> >      mClusterLengthLoc = ebmlLoc.offset;
> >      if (aFrame->GetFrameType() != EncodedFrame::FrameType::VORBIS_AUDIO_FRAME) {
> 
> Is this ever false? We're inside an aFrame->GetFrameType() == VP8_I_FRAME
> block.
> 
Yes, I'll remove it.
Blocks: 969290
Attachment #8406603 - Attachment is obsolete: true
Attachment #8410729 - Flags: review+
Attached patch bug-994557.part2.patch (obsolete) — Splinter Review
r=rillian
try server: https://tbpl.mozilla.org/?tree=Try&rev=1353611302b7
Attachment #8410730 - Flags: review+
fix nits.
Attachment #8410730 - Attachment is obsolete: true
Attachment #8410752 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/0930742ce5c6
https://hg.mozilla.org/mozilla-central/rev/80dba24cc929
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.