935827 - Implement MediaSegmentBase::ConstChunkIterator

Status: NEW

(Core :: Audio/Video: MediaStreamGraph, defect, P3)

Description

[u459114]

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

Comment 1

[John Lin [:jhlin][:jolin]]

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.

Comment 2

[u459114]

(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.

Comment 3

[u459114]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Still relevant?

Comment 5

[Paul Adenot (:padenot)]

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.

Comment 6

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.