Implement MediaSegmentBase::ConstChunkIterator

NEW
Unassigned

Status

()

Core
Audio/Video: MediaStreamGraph
P3
normal
Rank:
28
4 years ago
3 months ago

People

(Reporter: cjku, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
We only have MediaSegmentBase::ChunkIterator now.

It causes a problem when we receive a const MediaSegment from NotifyQueueTrackChanged callback. To access chunks, for reading frame size for example, we need to const_cast this MediaSegment. const_cast breaks the original design.

http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/TrackEncoder.cpp#l34
A dumb question from a C++ novice: why can't we just force ChunkIterator to only accept const MediaSegment in its constructor? Isn't iterating a mutable MediaSegment dangerous anyway? Enlighten me please.
(Reporter)

Comment 2

4 years ago
(In reply to John Lin[:jolin][:jhlin] from comment #1)
> A dumb question from a C++ novice: why can't we just force ChunkIterator to
> only accept const MediaSegment in its constructor? Isn't iterating a mutable
> MediaSegment dangerous anyway? Enlighten me please.
Through const iterator, you can only call const function of container and its element. Limit how a user uses container element by constant syntax.
In ConstChunkOperator, we can only return const Chunk element
  const Chunk& ConstChunkIterator::operator->()

In ChunkOperator, we should return Chunk element directly
  Chunk& ChunkIterator::operator->()

Give a const MediaSegment to listener object mean, we only allow this listener read information from this MediaSegement, and we are sure no context change in this MediaSegement after callback. This assumption is broken since we const_cast it.
(Reporter)

Comment 3

4 years ago
Another big problem in MediaRecorder pipeline is we AppendAndComumeChunk of callback media segment 

AudioTrackEncoder::NotifyQueuedTrackChanges(cosnt MediaSegment* aQueuedMedia)
AudioTrackEncoder::AppendAudioSegment(MediaSegment* aQueuedMedia) < aQueueMedia be const_cast to mutable pointer
  mRawSegment->AppendAndConsumeChunk(&chunk); 
        chunk->mBuffer = aChunk->mBuffer.forget(); 

Chunk in qQueuedMedia contains no buffer after NotifyQueuedTrackChanges. Which means any other track_change_listener can not read chunk data, since all the data had been move out by AudioTrackEncoder.

The reason that we can do it is because we const_cast MediaSegment to mutable pointer.
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
Still relevant?
Rank: 28
Flags: needinfo?(padenot)
Priority: -- → P2
Hm, I've changed this some, but I think the original concern is not addressed. This is still good to have, we can probably fold it in the MediaRecorder work that is going to happen soon.
Flags: needinfo?(padenot)
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.