No objection.
This reminds me that we have things to decoder configs that will trigger decoder flush/reset when the decoder itself really doesn't care. That's been true for a while (with addition of HDR and color space info). Any objection to detecting that condition upon config changes in DecoderStream and doing only a "soft" config change for things like rotation?
+sandersd
julien.isorce: Thank you for looking into this and propose the solution!
The fundamental question I have here is that: should VideoDecoderConfig contain only configs that the "decoder" needs, or should we put all video configs in this struct, e.g. those only needed to render the video correctly?
If the former, then we probably should not put video_ratation() in VideoDecoderConfig. Otherwise, we'll pass it all the way down to the decoder, and it'll simply be ignored.
The other option is to put all video related configs in VideoDecoderConfig, and use it everywhere, including in the decoders, as well as in the rendering path. This way it's more convenient for developers, but we are encapsulating less. Also, if we do so, VideoDecoderConfig should probably be renamed to VideoConfig.
Thx for the feedbacks.
@chcunningham: sure, could you point me to a particular unit test ?
@xhwang: I have the feeling that VideoDecoderConfig purpose has evolved from its initial purpose and should probably be renamed to VideoConfig. Btw, is VideoDecoderConfig supposed to change in the middle of the stream ? Or is it constant for one instance of DemuxerStream ?
I submitted https://chromium-review.googlesource.com/c/chromium/src/+/684740 . I think it makes it easier to comment on pros and cons. Please have a look, thx.
#5: VideoDecoderConfig can change at MSE segment boundaries, and results in VideoDecoder::Initialize() being called again. Typically this corresponds to a resolution change.
#6: oh good then the change will still allow "dynamic rotation change" if needed in the future. Speaking about MSE, note that currenty ChunkDemuxerStream::video_rotation() always return VIDEO_ROTATION_0.
> @chcunningham: sure, could you point me to a particular unit test ?
No unit tests demonstrate this AFAIK. For your CL (at least for the initial CL), I wouldn't worry with it.
In a separate CL, perhaps we can do the rename xhwang@ mentioned. At that time, we could introduce a new DecodeConfigMatches() function that would be used in decoder stream [0] to decide whether a full reset/flush is really necessary (it wouldn't be for just a rotation change).
But my proposal needs some buy in from xhwang/sandersd
[0] https://cs.chromium.org/chromium/src/media/filters/decoder_stream.cc?rcl=02b7e5fa7bc98300eebfb88192e66d1f1d2d0ba7&l=633
Comment 1 by dalecur...@chromium.org
, Sep 25 2017