1305347 - MSE: When Coded Frame Removal is called, all samples are always parsed

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

When frames are removed from the source buffer; all frames from the source buffer are parsed to determine if they are to be removed.

However, while frames are stored in decode order, once a frame is passed the end of the removal interval, we can stop the loop.

Following bug 1302573, the removal of frames code is called much more often. 
It's a cheao optimisations, especially as under most circumstances only frames located at the beginning of the sourcebuffer are removed.

Comment 1

[Jean-Yves Avenard [:jya]]

The description isn't quite actually correct.

Considering the following case.

0 2 1 3 5 4 6 8 7
|     |     |

The | indicates a keyframe.
Now say we want to remove frame 1.
We don't want to exit the loop after seeing frame 2 (it's time is passed the removal interval), otherwise the frame 1 won't be removed.

Another theoretical case would be:
0 2 1 3 5 4 6 8 7
|       |     |

say we want to remove frame 3 and 4. Per spec, frame 5 should be removed too as "Remove all possible decoding dependencies on the coded frames removed in the previous step by removing all coded frames from this track buffer between those frames removed in the previous step and the next random access point after those removed frames. ". Even though, in theory frame 5 doesn't have to be removed in any way being a keyframe. But since it's being between frame 3 and the next keyframe.

So what we could do, that would work even for this weird case (and likely non existent):
set a flag when we encounter a frame that is past our removal interval's end. That flag is reset when we find a frame that is contained in the interval.

When we encounter a new keyframe, if the flag is set, we may exit the loop.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1305347: [MSE] Break removal loop early when possible.

Review commit: https://reviewboard.mozilla.org/r/83842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83842/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8798322 [details]
Bug 1305347: [MSE] Break removal loop early when possible.

https://reviewboard.mozilla.org/r/83842/#review82436

r+, with or without my suggestion.

::: dom/media/mediasource/TrackBuffersManager.cpp:1768
(Diff revision 1)
>      if (aIntervals.Contains(sampleInterval)) {
>        if (firstRemovedIndex.isNothing()) {
>          firstRemovedIndex = Some(i);
>        }
>        lastRemovedIndex = i;
> +      mayBreakLoop = false;
> +    }
> +    if (sample->mKeyframe && mayBreakLoop) {
> +      break;
> +    }
> +    if (sampleInterval.mStart > intervalsEnd) {
> +      mayBreakLoop = true;
>      }

At the moment you have something like:
  if(i.contains(s)){b=0}
  if(k&&b){break}
  if(s>i){b=1}

If the 1st test succeeds:
- You will have mayBreakLoop=false and therefore the 2nd 'if' will always fail.
- aIntervals contains sample and therefore 'sample.start > interval.end' is not possible (right?)

If I'm correct, I think you could put an 'else' to bypass the other two tests if the first one succeeds:
  if(){
  } else {
    if(){}
    if(){}
  }

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8798322 [details]
Bug 1305347: [MSE] Break removal loop early when possible.

https://reviewboard.mozilla.org/r/83842/#review82436

> At the moment you have something like:
>   if(i.contains(s)){b=0}
>   if(k&&b){break}
>   if(s>i){b=1}
> 
> If the 1st test succeeds:
> - You will have mayBreakLoop=false and therefore the 2nd 'if' will always fail.
> - aIntervals contains sample and therefore 'sample.start > interval.end' is not possible (right?)
> 
> If I'm correct, I think you could put an 'else' to bypass the other two tests if the first one succeeds:
>   if(){
>   } else {
>     if(){}
>     if(){}
>   }

I think a single continue in the main if will do...

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8798322 [details]
Bug 1305347: [MSE] Break removal loop early when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83842/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32f1527481e8
[MSE] Break removal loop early when possible. r=gerald

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8798322 [details]
Bug 1305347: [MSE] Break removal loop early when possible.

https://reviewboard.mozilla.org/r/83842/#review82436

> I think a single continue in the main if will do...

Yes, same effect here.
I just have a personal aversion to continue's, that's why I recommended the 'else'. But go for it if you're happy with maintaining it. ;-)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32f1527481e8