[FFmpeg-devel] [PATCH] lavf/http: Add 301 and 503 error codes to http_write_reply()

[Stephan Holljes]

On Thu, Aug 20, 2015 at 2:39 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Wed, Aug 19, 2015 at 7:59 PM, Stephan Holljes
> <klaxa1337 at googlemail.com> wrote:
>> On Thu, Aug 20, 2015 at 12:11 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> On Wed, Aug 19, 2015 at 12:14 PM, Stephan Holljes
>>> <klaxa1337 at googlemail.com> wrote:
>>>> ---
>>>>  libavformat/http.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/libavformat/http.c b/libavformat/http.c
>>>> index a136918..4dbef3f 100644
>>>> --- a/libavformat/http.c
>>>> +++ b/libavformat/http.c
>>>> @@ -348,11 +348,19 @@ static int http_write_reply(URLContext* h, int status_code)
>>>>          reply_text = "OK";
>>>>          content_type = "application/octet-stream";
>>>>          break;
>>>> +    case 301:
>>>> +        reply_code = 301;
>>>> +        reply_text = "Moved Permanently";
>>>> +        break;
>>>
>>> 301 is usually used for URL redirection,
>>> and you don't seem to do anything beyond setting the message.
>>> There needs to be additional logic somewhere to handle this.
>>> Nevertheless, it is still ok as a patch to me,
>>> since I assume this will be handled later on.
>>> I strongly suggest adding something to clarify this in the commit message.
>>
>> I did not think about that and so far I have only used it in my
>> modified ffserver code. What makes a 301 reply different from the
>> other replies? Maybe I didn't understand the RFC correctly.
>
> Quoting from the RFC:
> "The new permanent URI SHOULD be given by the Location field in the response.
> Unless the request method was HEAD,
> the entity of the response SHOULD contain a short hypertext note with
> a hyperlink to the new URI(s). "
>
> My point is that somewhere in the response a new URI must be provided,
> so that the client can go to the redirected location.
> I don't know where/how such logic should go, but it needs to be done.
>

Ah yes, thus far the application had to take care of that. Using the
location field in HTTPContext seems like a good place to store this
information. I will work on a patch that includes this. Thanks!

>>
>>>
>>>>      case AVERROR_HTTP_SERVER_ERROR:
>>>>      case 500:
>>>>          reply_code = 500;
>>>>          reply_text = "Internal server error";
>>>>          break;
>>>> +    case 503:
>>>> +        reply_code = 503;
>>>> +        reply_text = "Service Unavailable";
>>>> +        break;
>>>
>>> Looks ok to me.
>>>
>>>>      default:
>>>>          return AVERROR(EINVAL);
>>>>      }
>>>> --
>>>> 2.1.0
>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel