Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only show video player error dialog if the error corresponds to the current slide #513

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Sep 2, 2020

Fix #437

tl;dr it doesn't make sense to show an error dialog for a video that is not on the screen anymore.

Been checking the error handling and I think it's in good shape so for the specific case described in the issue I think it's OK to present a Dialog. So instead of trying to silence or change any of that, what I did in this PR is:

  • elevate the error handling for video playing from BackgroundSurfaceManager up to the Activity through a new VidePlayerErrorListener interface
  • at the ComposeLoopFrameActivity level, we know about Story slides , so we can check whether an error coming from the video player corresponds to the current slide being presented on the screen by comparing the video player Uri and that of the current slide's BackgroundSource. Then if these match, we're good to show the dialog.

To test:

  • You can try with a corrupt video or alternatively, having a few slides and a couple of video slides and then switching slides frantically would eventually make the dialog appear, but only if the slide matches.
  • You could try this before and after: before, you'll get several dialogs. After, you'll get less dialogs.
  • Alternatively, you can add logs and observe how sometimes the error listener is called but the current slide differs and as such, the dialog showing code is not called.

…layerErrorListener interface to let the Activity level decide whether to show video playing error dialog depending on which slide we are currently on.
@mzorz mzorz requested a review from aforcier September 2, 2020 12:36
@aforcier aforcier self-assigned this Sep 8, 2020
@aforcier
Copy link
Collaborator

aforcier commented Sep 8, 2020

@mzorz the changes overall make sense but I'm not sure this is enough to resolve the issue in #513. Maybe I'm missing something.

I know you mentioned:

Been checking the error handling and I think it's in good shape so for the specific case described in the issue I think it's OK to present a Dialog.

But I'm not sure I understand why we want to present a Dialog there, since the video is not corrupt and in fact is playing just fine under the dialog. I can actually reproduce #513 without deleting any slide:

  1. Create story
  2. Using story camera capture a video (slide 1)
  3. Using story camera capture a photo or video (slide 2)
  4. Tap on slide 1 (the video)
  5. Dialog appears that says "error playing video" but slide 1 continues to playback \

I'm able to reproduce on a Pixel 3 (API 29), as well as a Pixel 3XL emulator (API 29)

@mzorz
Copy link
Contributor Author

mzorz commented Sep 9, 2020

But I'm not sure I understand why we want to present a Dialog there, since the video is not corrupt and in fact is playing just fine under the dialog. I can actually reproduce #513 without deleting any slide:

Thanks for providing steps to reproduce - I dug into this quite a bit yesterday and can see the video player is getting triggered twice: the first time it fails and as such the dialog is presented, the second time (which is immediate) gets called when the surface ready listener gets called on the VideoPlayerBasicHandling handler itself (this was there for the cases where you send the Activity to the background and then back to the foreground so it would start playing again).

That explains why the dialog appears (which is correct) and why the video is still played correctly while the error dialog sits on top, causing the confusion.

As per to the extent I could follow yesterday, the exact cause why it fails the first time and the dialog is shown is because of an IllegalStateException. I have been trying to make sense of which that state is when we stop the player by switching to another slide in the first place, but it doesn't make sense because we re-create the player instance each time we switch back to the slide (so it should be in a fresh state).

As discussed, going to dig more into this later as I switch priorities for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Error playing video" message when deleting a slide
2 participants